[PATCH net 0/2] net:usb:lan78xx: move LAN7800 internal phy register accesses to the phy driver.

[PATCH net 0/2] net:usb:lan78xx: move LAN7800 internal phy register accesses to the phy driver.

[Yuiko Oshino]

Move the LAN7800 internal phy register accesses from lan78xx.c to the new lan88xx_link_change_notify() in the microchip.c, the phy driver.

Yuiko Oshino (2):
  net:usb:lan78xx: fix accessing the LAN7800's internal phy specific
    registers from the MAC driver
  net:phy:microchip: fix accessing the LAN7800's internal phy specific
    registers from the MAC driver

 drivers/net/phy/microchip.c | 32 ++++++++++++++++++++++++++++++++
 drivers/net/usb/lan78xx.c   | 27 +--------------------------
 2 files changed, 33 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH net 1/2] 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).

Fixes: 14437e3fa284f465dbbc8611fd4331ca8d60e986 ("lan78xx: workaround of forced 100 Full/Half duplex mode error")
Signed-off-by: Yuiko Oshino <yuiko.oshino@microchip.com>
---
 drivers/net/usb/lan78xx.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

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 net 1/2] net:usb:lan78xx: fix accessing the LAN7800's internal phy specific registers from the MAC driver

[Andrew Lunn]

On Thu, Feb 16, 2023 at 07:20:53AM -0700, Yuiko Oshino wrote:
> Move the LAN7800 internal phy (phy ID  0x0007c132) specific register accesses to the phy driver (microchip.c).
> 
> Fixes: 14437e3fa284f465dbbc8611fd4331ca8d60e986 ("lan78xx: workaround of forced 100 Full/Half duplex mode error")

I would not say this is a fix which needs putting into stable. The
architecture is wrong to do it in the MAC driver, but i don't think it
causes real issues?

So please submit this to net-next, not net.

   Andrew

RE: [PATCH net 1/2] 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: Thursday, February 16, 2023 10:09 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 net 1/2] 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 Thu, Feb 16, 2023 at 07:20:53AM -0700, Yuiko Oshino wrote:
>> Move the LAN7800 internal phy (phy ID  0x0007c132) specific register accesses
>to the phy driver (microchip.c).
>>
>> Fixes: 14437e3fa284f465dbbc8611fd4331ca8d60e986 ("lan78xx: workaround
>> of forced 100 Full/Half duplex mode error")
>
>I would not say this is a fix which needs putting into stable. The architecture is
>wrong to do it in the MAC driver, but i don't think it causes real issues?
>
>So please submit this to net-next, not net.
>
>   Andrew

Hi Andrew,

Enguerrand reported the below on 20 Dec 2022, therefore, submitting to net.

" 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 unapropriate behavior."

Thank you.
Yuiko

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

[Andrew Lunn]

> Hi Andrew,
> 
> Enguerrand reported the below on 20 Dec 2022, therefore, submitting to net.
> 
> " 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 unapropriate behavior."

O.K.

So please include in the commit message this information. Then it
becomes clear it really is a fix.

Also, you did not add a fixes: tag to the second patch. There is a
danger the first patch gets back ported, but not the second. Since you
are just moving code around, i suggest you have just have one patch.

    Andrew

RE: [PATCH net 1/2] 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: Thursday, February 16, 2023 11:56 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 net 1/2] 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
>
>> Hi Andrew,
>>
>> Enguerrand reported the below on 20 Dec 2022, therefore, submitting to net.
>>
>> " 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 unapropriate behavior."
>
>O.K.
>
>So please include in the commit message this information. Then it becomes clear it
>really is a fix.
>
>Also, you did not add a fixes: tag to the second patch. There is a danger the first
>patch gets back ported, but not the second. Since you are just moving code around,
>i suggest you have just have one patch.
>
>    Andrew

No problem.
One patch would be easier!
I will add the comment from Enguerrand.
I will do v2 tomorrow.

Thank you.
Yuiko

[PATCH net 2/2] net:phy:microchip: fix accessing the LAN7800's internal phy specific registers from the MAC driver

[Yuiko Oshino]

Add lan88xx_link_change_notify() to the internal phy driver and move the phy specific register accesses there.

Signed-off-by: Yuiko Oshino <yuiko.oshino@microchip.com>
---
 drivers/net/phy/microchip.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

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,
-- 
2.17.1