[PATCH v2 net] net:usb:lan78xx: fix accessing the LAN7800's internal phy specific registers from the MAC driver

[PATCH v2 net] net:usb:lan78xx: fix accessing the LAN7800's internal phy specific registers from the MAC driver

[Yuiko Oshino]

Move the LAN7800 internal phy (phy ID  0x0007c132) specific register accesses to the phy driver (microchip.c).

Fix the error reported by Enguerrand de Ribaucourt in December 2022,
"Some operations during the cable switch workaround modify the register
LAN88XX_INT_MASK of the PHY. However, this register is specific to the
LAN8835 PHY. For instance, if a DP8322I PHY is connected to the LAN7801,
that register (0x19), corresponds to the LED and MAC address
configuration, resulting in inappropriate behavior."

I did not test with the DP8322I PHY, but I tested with an EVB-LAN7800 with the internal PHY.

Fixes: 14437e3fa284f465dbbc8611fd4331ca8d60e986 ("lan78xx: workaround of forced 100 Full/Half duplex mode error")
Signed-off-by: Yuiko Oshino <yuiko.oshino@microchip.com>

v1->v2: 
    - Add an error behavior comment reported by Enguerrand.
    - Create a single patch instead of a series.
---
 drivers/net/phy/microchip.c | 32 ++++++++++++++++++++++++++++++++
 drivers/net/usb/lan78xx.c   | 27 +--------------------------
 2 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
index ccecee2524ce..0b88635f4fbc 100644
--- a/drivers/net/phy/microchip.c
+++ b/drivers/net/phy/microchip.c
@@ -342,6 +342,37 @@ static int lan88xx_config_aneg(struct phy_device *phydev)
 	return genphy_config_aneg(phydev);
 }
 
+static void lan88xx_link_change_notify(struct phy_device *phydev)
+{
+	int temp;
+
+	/* At forced 100 F/H mode, chip may fail to set mode correctly
+	 * when cable is switched between long(~50+m) and short one.
+	 * As workaround, set to 10 before setting to 100
+	 * at forced 100 F/H mode.
+	 */
+	if (!phydev->autoneg && phydev->speed == 100) {
+		/* disable phy interrupt */
+		temp = phy_read(phydev, LAN88XX_INT_MASK);
+		temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_;
+		phy_write(phydev, LAN88XX_INT_MASK, temp);
+
+		temp = phy_read(phydev, MII_BMCR);
+		temp &= ~(BMCR_SPEED100 | BMCR_SPEED1000);
+		phy_write(phydev, MII_BMCR, temp); /* set to 10 first */
+		temp |= BMCR_SPEED100;
+		phy_write(phydev, MII_BMCR, temp); /* set to 100 later */
+
+		/* clear pending interrupt generated while workaround */
+		temp = phy_read(phydev, LAN88XX_INT_STS);
+
+		/* enable phy interrupt back */
+		temp = phy_read(phydev, LAN88XX_INT_MASK);
+		temp |= LAN88XX_INT_MASK_MDINTPIN_EN_;
+		phy_write(phydev, LAN88XX_INT_MASK, temp);
+	}
+}
+
 static struct phy_driver microchip_phy_driver[] = {
 {
 	.phy_id		= 0x0007c132,
@@ -359,6 +390,7 @@ static struct phy_driver microchip_phy_driver[] = {
 
 	.config_init	= lan88xx_config_init,
 	.config_aneg	= lan88xx_config_aneg,
+	.link_change_notify = lan88xx_link_change_notify,
 
 	.config_intr	= lan88xx_phy_config_intr,
 	.handle_interrupt = lan88xx_handle_interrupt,
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index f18ab8e220db..068488890d57 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2115,33 +2115,8 @@ static void lan78xx_remove_mdio(struct lan78xx_net *dev)
 static void lan78xx_link_status_change(struct net_device *net)
 {
 	struct phy_device *phydev = net->phydev;
-	int temp;
-
-	/* At forced 100 F/H mode, chip may fail to set mode correctly
-	 * when cable is switched between long(~50+m) and short one.
-	 * As workaround, set to 10 before setting to 100
-	 * at forced 100 F/H mode.
-	 */
-	if (!phydev->autoneg && (phydev->speed == 100)) {
-		/* disable phy interrupt */
-		temp = phy_read(phydev, LAN88XX_INT_MASK);
-		temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_;
-		phy_write(phydev, LAN88XX_INT_MASK, temp);
 
-		temp = phy_read(phydev, MII_BMCR);
-		temp &= ~(BMCR_SPEED100 | BMCR_SPEED1000);
-		phy_write(phydev, MII_BMCR, temp); /* set to 10 first */
-		temp |= BMCR_SPEED100;
-		phy_write(phydev, MII_BMCR, temp); /* set to 100 later */
-
-		/* clear pending interrupt generated while workaround */
-		temp = phy_read(phydev, LAN88XX_INT_STS);
-
-		/* enable phy interrupt back */
-		temp = phy_read(phydev, LAN88XX_INT_MASK);
-		temp |= LAN88XX_INT_MASK_MDINTPIN_EN_;
-		phy_write(phydev, LAN88XX_INT_MASK, temp);
-	}
+	phy_print_status(phydev);
 }
 
 static int irq_map(struct irq_domain *d, unsigned int irq,
-- 
2.17.1

Re: [PATCH v2 net] net:usb:lan78xx: fix accessing the LAN7800's internal phy specific registers from the MAC driver

[Andrew Lunn]

On Fri, Feb 17, 2023 at 06:09:00AM -0700, Yuiko Oshino wrote:
> Move the LAN7800 internal phy (phy ID  0x0007c132) specific register accesses to the phy driver (microchip.c).
> 
> Fix the error reported by Enguerrand de Ribaucourt in December 2022,
> "Some operations during the cable switch workaround modify the register
> LAN88XX_INT_MASK of the PHY. However, this register is specific to the
> LAN8835 PHY. For instance, if a DP8322I PHY is connected to the LAN7801,
> that register (0x19), corresponds to the LED and MAC address
> configuration, resulting in inappropriate behavior."
> 
> I did not test with the DP8322I PHY, but I tested with an EVB-LAN7800 with the internal PHY.

Please keep you commit message lines less than 80 characters.

> Fixes: 14437e3fa284f465dbbc8611fd4331ca8d60e986 ("lan78xx: workaround of forced 100 Full/Half duplex mode error")

Please use the short hash, not the long.

The Fixes: tag is an exception to the 80 characters rule, it can be as
long as it needs to be.

Otherwise this patch looks good now.

	  Andrew

RE: [PATCH v2 net] net:usb:lan78xx: fix accessing the LAN7800's internal phy specific registers from the MAC driver

[Yuiko.Oshino]

>-----Original Message-----
>From: Andrew Lunn <andrew@lunn.ch>
>Sent: Friday, February 17, 2023 9:23 AM
>To: Yuiko Oshino - C18177 <Yuiko.Oshino@microchip.com>
>Cc: enguerrand.de-ribaucourt@savoirfairelinux.com; Woojung Huh - C21699
><Woojung.Huh@microchip.com>; hkallweit1@gmail.com; netdev@vger.kernel.org;
>pabeni@redhat.com; davem@davemloft.net; UNGLinuxDriver
><UNGLinuxDriver@microchip.com>; linux@armlinux.org.uk; edumazet@google.com;
>linux-usb@vger.kernel.org; kuba@kernel.org
>Subject: Re: [PATCH v2 net] net:usb:lan78xx: fix accessing the LAN7800's internal
>phy specific registers from the MAC driver
>
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>content is safe
>
>On Fri, Feb 17, 2023 at 06:09:00AM -0700, Yuiko Oshino wrote:
>> Move the LAN7800 internal phy (phy ID  0x0007c132) specific register accesses to
>the phy driver (microchip.c).
>>
>> Fix the error reported by Enguerrand de Ribaucourt in December 2022,
>> "Some operations during the cable switch workaround modify the
>> register LAN88XX_INT_MASK of the PHY. However, this register is
>> specific to the
>> LAN8835 PHY. For instance, if a DP8322I PHY is connected to the
>> LAN7801, that register (0x19), corresponds to the LED and MAC address
>> configuration, resulting in inappropriate behavior."
>>
>> I did not test with the DP8322I PHY, but I tested with an EVB-LAN7800 with the
>internal PHY.
>
>Please keep you commit message lines less than 80 characters.
>
>> Fixes: 14437e3fa284f465dbbc8611fd4331ca8d60e986 ("lan78xx: workaround
>> of forced 100 Full/Half duplex mode error")
>
>Please use the short hash, not the long.
>
>The Fixes: tag is an exception to the 80 characters rule, it can be as long as it needs
>to be.
>
>Otherwise this patch looks good now.
>
>          Andrew

Thank you, Andrew!

I will do v3 shortly.

Yuiko