[PATCH net-next] net: phy: improve handling link_change_notify callback

[PATCH net-next] net: phy: improve handling link_change_notify callback

[Heiner Kallweit]

Currently the Phy driver's link_change_notify callback is called
whenever the state machine is run (every second if polling), no matter
whether the state changed or not. This isn't needed and may confuse
users considering the name of the callback. Therefore let's change
the behavior and call this callback only in case of an actual
state change.

This requires changes to the at803x and rockchip drivers.
at803x can be simplified so that it reacts on a state change to
PHY_NOLINK only.
The rockchip driver can also be much simplified. We simply re-init
the AFE/DSP registers whenever we change to PHY_RUNNING and speed
is 100Mbps. This causes very small overhead because we do this even
if the speed was 100Mbps already. But this is neglectable and
I think justified by the much simpler code.

Changes are compile-tested only.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/at803x.c   | 26 +++++++++-----------------
 drivers/net/phy/phy.c      |  8 ++++----
 drivers/net/phy/rockchip.c | 31 ++-----------------------------
 3 files changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f3e96191e..f315ab468 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -324,8 +324,6 @@ static int at803x_config_intr(struct phy_device *phydev)
 
 static void at803x_link_change_notify(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
-
 	/*
 	 * Conduct a hardware reset for AT8030 every time a link loss is
 	 * signalled. This is necessary to circumvent a hardware bug that
@@ -333,25 +331,19 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	 * in the FIFO. In such cases, the FIFO enters an error mode it
 	 * cannot recover from by software.
 	 */
-	if (phydev->state == PHY_NOLINK) {
-		if (phydev->mdio.reset && !priv->phy_reset) {
-			struct at803x_context context;
+	if (phydev->state == PHY_NOLINK && phydev->mdio.reset) {
+		struct at803x_context context;
 
-			at803x_context_save(phydev, &context);
+		at803x_context_save(phydev, &context);
 
-			phy_device_reset(phydev, 1);
-			msleep(1);
-			phy_device_reset(phydev, 0);
-			msleep(1);
+		phy_device_reset(phydev, 1);
+		msleep(1);
+		phy_device_reset(phydev, 0);
+		msleep(1);
 
-			at803x_context_restore(phydev, &context);
+		at803x_context_restore(phydev, &context);
 
-			phydev_dbg(phydev, "%s(): phy was reset\n",
-				   __func__);
-			priv->phy_reset = true;
-		}
-	} else {
-		priv->phy_reset = false;
+		phydev_dbg(phydev, "%s(): phy was reset\n", __func__);
 	}
 }
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3745220c5..5938c5acf 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -891,9 +891,6 @@ void phy_state_machine(struct work_struct *work)
 
 	old_state = phydev->state;
 
-	if (phydev->drv && phydev->drv->link_change_notify)
-		phydev->drv->link_change_notify(phydev);
-
 	switch (phydev->state) {
 	case PHY_DOWN:
 	case PHY_READY:
@@ -940,10 +937,13 @@ void phy_state_machine(struct work_struct *work)
 	if (err < 0)
 		phy_error(phydev);
 
-	if (old_state != phydev->state)
+	if (old_state != phydev->state) {
 		phydev_dbg(phydev, "PHY state change %s -> %s\n",
 			   phy_state_to_str(old_state),
 			   phy_state_to_str(phydev->state));
+		if (phydev->drv && phydev->drv->link_change_notify)
+			phydev->drv->link_change_notify(phydev);
+	}
 
 	/* Only re-schedule a PHY state machine change if we are polling the
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
diff --git a/drivers/net/phy/rockchip.c b/drivers/net/phy/rockchip.c
index 95abf7072..9053b1d01 100644
--- a/drivers/net/phy/rockchip.c
+++ b/drivers/net/phy/rockchip.c
@@ -104,41 +104,14 @@ static int rockchip_integrated_phy_config_init(struct phy_device *phydev)
 
 static void rockchip_link_change_notify(struct phy_device *phydev)
 {
-	int speed = SPEED_10;
-
-	if (phydev->autoneg == AUTONEG_ENABLE) {
-		int reg = phy_read(phydev, MII_SPECIAL_CONTROL_STATUS);
-
-		if (reg < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", reg);
-			return;
-		}
-
-		if (reg & MII_SPEED_100)
-			speed = SPEED_100;
-		else if (reg & MII_SPEED_10)
-			speed = SPEED_10;
-	} else {
-		int bmcr = phy_read(phydev, MII_BMCR);
-
-		if (bmcr < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", bmcr);
-			return;
-		}
-
-		if (bmcr & BMCR_SPEED100)
-			speed = SPEED_100;
-		else
-			speed = SPEED_10;
-	}
-
 	/*
 	 * If mode switch happens from 10BT to 100BT, all DSP/AFE
 	 * registers are set to default values. So any AFE/DSP
 	 * registers have to be re-initialized in this case.
 	 */
-	if ((phydev->speed == SPEED_10) && (speed == SPEED_100)) {
+	if (phydev->state == PHY_RUNNING && phydev->speed == SPEED_100) {
 		int ret = rockchip_integrated_phy_analog_init(phydev);
+
 		if (ret)
 			phydev_err(phydev, "rockchip_integrated_phy_analog_init err: %d.\n",
 				   ret);
-- 
2.21.0

[PATCH net-next] net: phy: improve handling link_change_notify callback

[Heiner Kallweit]

Currently the Phy driver's link_change_notify callback is called
whenever the state machine is run (every second if polling), no matter
whether the state changed or not. This isn't needed and may confuse
users considering the name of the callback. Therefore let's change
the behavior and call this callback only in case of an actual
state change.

This requires changes to the at803x and rockchip drivers.
at803x can be simplified so that it reacts on a state change to
PHY_NOLINK only.
The rockchip driver can also be much simplified. We simply re-init
the AFE/DSP registers whenever we change to PHY_RUNNING and speed
is 100Mbps. This causes very small overhead because we do this even
if the speed was 100Mbps already. But this is neglectable and
I think justified by the much simpler code.

Changes are compile-tested only.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/at803x.c   | 26 +++++++++-----------------
 drivers/net/phy/phy.c      |  8 ++++----
 drivers/net/phy/rockchip.c | 31 ++-----------------------------
 3 files changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f3e96191e..f315ab468 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -324,8 +324,6 @@ static int at803x_config_intr(struct phy_device *phydev)
 
 static void at803x_link_change_notify(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
-
 	/*
 	 * Conduct a hardware reset for AT8030 every time a link loss is
 	 * signalled. This is necessary to circumvent a hardware bug that
@@ -333,25 +331,19 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	 * in the FIFO. In such cases, the FIFO enters an error mode it
 	 * cannot recover from by software.
 	 */
-	if (phydev->state == PHY_NOLINK) {
-		if (phydev->mdio.reset && !priv->phy_reset) {
-			struct at803x_context context;
+	if (phydev->state == PHY_NOLINK && phydev->mdio.reset) {
+		struct at803x_context context;
 
-			at803x_context_save(phydev, &context);
+		at803x_context_save(phydev, &context);
 
-			phy_device_reset(phydev, 1);
-			msleep(1);
-			phy_device_reset(phydev, 0);
-			msleep(1);
+		phy_device_reset(phydev, 1);
+		msleep(1);
+		phy_device_reset(phydev, 0);
+		msleep(1);
 
-			at803x_context_restore(phydev, &context);
+		at803x_context_restore(phydev, &context);
 
-			phydev_dbg(phydev, "%s(): phy was reset\n",
-				   __func__);
-			priv->phy_reset = true;
-		}
-	} else {
-		priv->phy_reset = false;
+		phydev_dbg(phydev, "%s(): phy was reset\n", __func__);
 	}
 }
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3745220c5..5938c5acf 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -891,9 +891,6 @@ void phy_state_machine(struct work_struct *work)
 
 	old_state = phydev->state;
 
-	if (phydev->drv && phydev->drv->link_change_notify)
-		phydev->drv->link_change_notify(phydev);
-
 	switch (phydev->state) {
 	case PHY_DOWN:
 	case PHY_READY:
@@ -940,10 +937,13 @@ void phy_state_machine(struct work_struct *work)
 	if (err < 0)
 		phy_error(phydev);
 
-	if (old_state != phydev->state)
+	if (old_state != phydev->state) {
 		phydev_dbg(phydev, "PHY state change %s -> %s\n",
 			   phy_state_to_str(old_state),
 			   phy_state_to_str(phydev->state));
+		if (phydev->drv && phydev->drv->link_change_notify)
+			phydev->drv->link_change_notify(phydev);
+	}
 
 	/* Only re-schedule a PHY state machine change if we are polling the
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
diff --git a/drivers/net/phy/rockchip.c b/drivers/net/phy/rockchip.c
index 95abf7072..9053b1d01 100644
--- a/drivers/net/phy/rockchip.c
+++ b/drivers/net/phy/rockchip.c
@@ -104,41 +104,14 @@ static int rockchip_integrated_phy_config_init(struct phy_device *phydev)
 
 static void rockchip_link_change_notify(struct phy_device *phydev)
 {
-	int speed = SPEED_10;
-
-	if (phydev->autoneg == AUTONEG_ENABLE) {
-		int reg = phy_read(phydev, MII_SPECIAL_CONTROL_STATUS);
-
-		if (reg < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", reg);
-			return;
-		}
-
-		if (reg & MII_SPEED_100)
-			speed = SPEED_100;
-		else if (reg & MII_SPEED_10)
-			speed = SPEED_10;
-	} else {
-		int bmcr = phy_read(phydev, MII_BMCR);
-
-		if (bmcr < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", bmcr);
-			return;
-		}
-
-		if (bmcr & BMCR_SPEED100)
-			speed = SPEED_100;
-		else
-			speed = SPEED_10;
-	}
-
 	/*
 	 * If mode switch happens from 10BT to 100BT, all DSP/AFE
 	 * registers are set to default values. So any AFE/DSP
 	 * registers have to be re-initialized in this case.
 	 */
-	if ((phydev->speed == SPEED_10) && (speed == SPEED_100)) {
+	if (phydev->state == PHY_RUNNING && phydev->speed == SPEED_100) {
 		int ret = rockchip_integrated_phy_analog_init(phydev);
+
 		if (ret)
 			phydev_err(phydev, "rockchip_integrated_phy_analog_init err: %d.\n",
 				   ret);
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 3 Mar 2019 19:58:57 +0100

> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Therefore let's change
> the behavior and call this callback only in case of an actual
> state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is neglectable and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Someone please review this.

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 3 Mar 2019 19:58:57 +0100

> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Therefore let's change
> the behavior and call this callback only in case of an actual
> state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is neglectable and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Someone please review this.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Andrew Lunn]

On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Sun, 3 Mar 2019 19:58:57 +0100
> 
> > Currently the Phy driver's link_change_notify callback is called
> > whenever the state machine is run (every second if polling), no matter
> > whether the state changed or not. This isn't needed and may confuse
> > users considering the name of the callback. Therefore let's change
> > the behavior and call this callback only in case of an actual
> > state change.
> > 
> > This requires changes to the at803x and rockchip drivers.
> > at803x can be simplified so that it reacts on a state change to
> > PHY_NOLINK only.
> > The rockchip driver can also be much simplified. We simply re-init
> > the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> > is 100Mbps. This causes very small overhead because we do this even
> > if the speed was 100Mbps already. But this is neglectable and
> > I think justified by the much simpler code.
> > 
> > Changes are compile-tested only.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Someone please review this.

Hi David

We should probably wait for a Tested-by: from Daniel Mack
<zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
equivalent.

	Andrew

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Andrew Lunn]

On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Sun, 3 Mar 2019 19:58:57 +0100
> 
> > Currently the Phy driver's link_change_notify callback is called
> > whenever the state machine is run (every second if polling), no matter
> > whether the state changed or not. This isn't needed and may confuse
> > users considering the name of the callback. Therefore let's change
> > the behavior and call this callback only in case of an actual
> > state change.
> > 
> > This requires changes to the at803x and rockchip drivers.
> > at803x can be simplified so that it reacts on a state change to
> > PHY_NOLINK only.
> > The rockchip driver can also be much simplified. We simply re-init
> > the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> > is 100Mbps. This causes very small overhead because we do this even
> > if the speed was 100Mbps already. But this is neglectable and
> > I think justified by the much simpler code.
> > 
> > Changes are compile-tested only.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Someone please review this.

Hi David

We should probably wait for a Tested-by: from Daniel Mack
<zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
equivalent.

	Andrew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 4 Mar 2019 21:06:30 +0100

> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
>> Someone please review this.
> 
> We should probably wait for a Tested-by: from Daniel Mack
> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
> equivalent.

Ok.

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Mon, 4 Mar 2019 21:06:30 +0100

> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
>> Someone please review this.
> 
> We should probably wait for a Tested-by: from Daniel Mack
> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
> equivalent.

Ok.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 3 Mar 2019 19:58:57 +0100

> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Therefore let's change
> the behavior and call this callback only in case of an actual
> state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is neglectable and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

This is still rotting... I've marked it as deferred in patchwork.

This is the kind of patch review and testing scenerio that simply
drives me crazy as a maintainer.

Patches should never rot.

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 3 Mar 2019 19:58:57 +0100

> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Therefore let's change
> the behavior and call this callback only in case of an actual
> state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is neglectable and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

This is still rotting... I've marked it as deferred in patchwork.

This is the kind of patch review and testing scenerio that simply
drives me crazy as a maintainer.

Patches should never rot.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Heiko Stuebner]

Hi Andrew, Heiner,

Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
> > From: Heiner Kallweit <hkallweit1@gmail.com>
> > Date: Sun, 3 Mar 2019 19:58:57 +0100
> > 
> > > Currently the Phy driver's link_change_notify callback is called
> > > whenever the state machine is run (every second if polling), no matter
> > > whether the state changed or not. This isn't needed and may confuse
> > > users considering the name of the callback. Therefore let's change
> > > the behavior and call this callback only in case of an actual
> > > state change.
> > > 
> > > This requires changes to the at803x and rockchip drivers.
> > > at803x can be simplified so that it reacts on a state change to
> > > PHY_NOLINK only.
> > > The rockchip driver can also be much simplified. We simply re-init
> > > the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> > > is 100Mbps. This causes very small overhead because we do this even
> > > if the speed was 100Mbps already. But this is neglectable and
> > > I think justified by the much simpler code.
> > > 
> > > Changes are compile-tested only.
> > > 
> > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > 
> > Someone please review this.
> 
> Hi David
> 
> We should probably wait for a Tested-by: from Daniel Mack
> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
> equivalent.

We should probably add them to the list of recipients if we want
tests from them, which I've done now.

@David: patch in question that changes the Rockchip eth-phy is
https://patchwork.kernel.org/patch/10837217/

Sadly I don't have matching hardware to test this myself.


Heiko

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Heiko Stuebner]

Hi Andrew, Heiner,

Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
> > From: Heiner Kallweit <hkallweit1@gmail.com>
> > Date: Sun, 3 Mar 2019 19:58:57 +0100
> > 
> > > Currently the Phy driver's link_change_notify callback is called
> > > whenever the state machine is run (every second if polling), no matter
> > > whether the state changed or not. This isn't needed and may confuse
> > > users considering the name of the callback. Therefore let's change
> > > the behavior and call this callback only in case of an actual
> > > state change.
> > > 
> > > This requires changes to the at803x and rockchip drivers.
> > > at803x can be simplified so that it reacts on a state change to
> > > PHY_NOLINK only.
> > > The rockchip driver can also be much simplified. We simply re-init
> > > the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> > > is 100Mbps. This causes very small overhead because we do this even
> > > if the speed was 100Mbps already. But this is neglectable and
> > > I think justified by the much simpler code.
> > > 
> > > Changes are compile-tested only.
> > > 
> > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > 
> > Someone please review this.
> 
> Hi David
> 
> We should probably wait for a Tested-by: from Daniel Mack
> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
> equivalent.

We should probably add them to the list of recipients if we want
tests from them, which I've done now.

@David: patch in question that changes the Rockchip eth-phy is
https://patchwork.kernel.org/patch/10837217/

Sadly I don't have matching hardware to test this myself.


Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Daniel Mack]

On 12/3/2019 1:27 PM, Heiko Stuebner wrote:
> Hi Andrew, Heiner,
> 
> Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
>> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>> Date: Sun, 3 Mar 2019 19:58:57 +0100
>>>
>>>> Currently the Phy driver's link_change_notify callback is called
>>>> whenever the state machine is run (every second if polling), no matter
>>>> whether the state changed or not. This isn't needed and may confuse
>>>> users considering the name of the callback. Therefore let's change
>>>> the behavior and call this callback only in case of an actual
>>>> state change.
>>>>
>>>> This requires changes to the at803x and rockchip drivers.
>>>> at803x can be simplified so that it reacts on a state change to
>>>> PHY_NOLINK only.
>>>> The rockchip driver can also be much simplified. We simply re-init
>>>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
>>>> is 100Mbps. This causes very small overhead because we do this even
>>>> if the speed was 100Mbps already. But this is neglectable and
>>>> I think justified by the much simpler code.
>>>>
>>>> Changes are compile-tested only.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>
>>> Someone please review this.
>>
>> Hi David
>>
>> We should probably wait for a Tested-by: from Daniel Mack
>> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
>> equivalent.
> 
> We should probably add them to the list of recipients if we want
> tests from them, which I've done now.
> 
> @David: patch in question that changes the Rockchip eth-phy is
> https://patchwork.kernel.org/patch/10837217/
> 
> Sadly I don't have matching hardware to test this myself.

Thanks for considering me, but I don't have access to that hardware
either right now.

The changes look straight forward to me however, so just go ahead an
apply this. Once I get back to that setup, I'll report in case of a
regression.


Thanks,
Dainel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Daniel Mack]

On 12/3/2019 1:27 PM, Heiko Stuebner wrote:
> Hi Andrew, Heiner,
> 
> Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
>> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>> Date: Sun, 3 Mar 2019 19:58:57 +0100
>>>
>>>> Currently the Phy driver's link_change_notify callback is called
>>>> whenever the state machine is run (every second if polling), no matter
>>>> whether the state changed or not. This isn't needed and may confuse
>>>> users considering the name of the callback. Therefore let's change
>>>> the behavior and call this callback only in case of an actual
>>>> state change.
>>>>
>>>> This requires changes to the at803x and rockchip drivers.
>>>> at803x can be simplified so that it reacts on a state change to
>>>> PHY_NOLINK only.
>>>> The rockchip driver can also be much simplified. We simply re-init
>>>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
>>>> is 100Mbps. This causes very small overhead because we do this even
>>>> if the speed was 100Mbps already. But this is neglectable and
>>>> I think justified by the much simpler code.
>>>>
>>>> Changes are compile-tested only.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>
>>> Someone please review this.
>>
>> Hi David
>>
>> We should probably wait for a Tested-by: from Daniel Mack
>> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
>> equivalent.
> 
> We should probably add them to the list of recipients if we want
> tests from them, which I've done now.
> 
> @David: patch in question that changes the Rockchip eth-phy is
> https://patchwork.kernel.org/patch/10837217/
> 
> Sadly I don't have matching hardware to test this myself.

Thanks for considering me, but I don't have access to that hardware
either right now.

The changes look straight forward to me however, so just go ahead an
apply this. Once I get back to that setup, I'll report in case of a
regression.


Thanks,
Dainel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Florian Fainelli]

On 3/3/19 10:58 AM, Heiner Kallweit wrote:
> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Therefore let's change
> the behavior and call this callback only in case of an actual
> state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is neglectable and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

While you are it, we should probably audit all drivers making use of
PHYLIB and see whether the logic to compare the previous link with the
current link parameters is still necessary, and if it is, we should
consider moving that to the core PHYLIB.

> ---
>  drivers/net/phy/at803x.c   | 26 +++++++++-----------------
>  drivers/net/phy/phy.c      |  8 ++++----
>  drivers/net/phy/rockchip.c | 31 ++-----------------------------
>  3 files changed, 15 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index f3e96191e..f315ab468 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -324,8 +324,6 @@ static int at803x_config_intr(struct phy_device *phydev)
>  
>  static void at803x_link_change_notify(struct phy_device *phydev)
>  {
> -	struct at803x_priv *priv = phydev->priv;
> -
>  	/*
>  	 * Conduct a hardware reset for AT8030 every time a link loss is
>  	 * signalled. This is necessary to circumvent a hardware bug that
> @@ -333,25 +331,19 @@ static void at803x_link_change_notify(struct phy_device *phydev)
>  	 * in the FIFO. In such cases, the FIFO enters an error mode it
>  	 * cannot recover from by software.
>  	 */
> -	if (phydev->state == PHY_NOLINK) {
> -		if (phydev->mdio.reset && !priv->phy_reset) {
> -			struct at803x_context context;
> +	if (phydev->state == PHY_NOLINK && phydev->mdio.reset) {
> +		struct at803x_context context;
>  
> -			at803x_context_save(phydev, &context);
> +		at803x_context_save(phydev, &context);
>  
> -			phy_device_reset(phydev, 1);
> -			msleep(1);
> -			phy_device_reset(phydev, 0);
> -			msleep(1);
> +		phy_device_reset(phydev, 1);
> +		msleep(1);
> +		phy_device_reset(phydev, 0);
> +		msleep(1);
>  
> -			at803x_context_restore(phydev, &context);
> +		at803x_context_restore(phydev, &context);
>  
> -			phydev_dbg(phydev, "%s(): phy was reset\n",
> -				   __func__);
> -			priv->phy_reset = true;
> -		}
> -	} else {
> -		priv->phy_reset = false;
> +		phydev_dbg(phydev, "%s(): phy was reset\n", __func__);
>  	}
>  }
>  
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 3745220c5..5938c5acf 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -891,9 +891,6 @@ void phy_state_machine(struct work_struct *work)
>  
>  	old_state = phydev->state;
>  
> -	if (phydev->drv && phydev->drv->link_change_notify)
> -		phydev->drv->link_change_notify(phydev);
> -
>  	switch (phydev->state) {
>  	case PHY_DOWN:
>  	case PHY_READY:
> @@ -940,10 +937,13 @@ void phy_state_machine(struct work_struct *work)
>  	if (err < 0)
>  		phy_error(phydev);
>  
> -	if (old_state != phydev->state)
> +	if (old_state != phydev->state) {
>  		phydev_dbg(phydev, "PHY state change %s -> %s\n",
>  			   phy_state_to_str(old_state),
>  			   phy_state_to_str(phydev->state));
> +		if (phydev->drv && phydev->drv->link_change_notify)
> +			phydev->drv->link_change_notify(phydev);
> +	}
>  
>  	/* Only re-schedule a PHY state machine change if we are polling the
>  	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
> diff --git a/drivers/net/phy/rockchip.c b/drivers/net/phy/rockchip.c
> index 95abf7072..9053b1d01 100644
> --- a/drivers/net/phy/rockchip.c
> +++ b/drivers/net/phy/rockchip.c
> @@ -104,41 +104,14 @@ static int rockchip_integrated_phy_config_init(struct phy_device *phydev)
>  
>  static void rockchip_link_change_notify(struct phy_device *phydev)
>  {
> -	int speed = SPEED_10;
> -
> -	if (phydev->autoneg == AUTONEG_ENABLE) {
> -		int reg = phy_read(phydev, MII_SPECIAL_CONTROL_STATUS);
> -
> -		if (reg < 0) {
> -			phydev_err(phydev, "phy_read err: %d.\n", reg);
> -			return;
> -		}
> -
> -		if (reg & MII_SPEED_100)
> -			speed = SPEED_100;
> -		else if (reg & MII_SPEED_10)
> -			speed = SPEED_10;
> -	} else {
> -		int bmcr = phy_read(phydev, MII_BMCR);
> -
> -		if (bmcr < 0) {
> -			phydev_err(phydev, "phy_read err: %d.\n", bmcr);
> -			return;
> -		}
> -
> -		if (bmcr & BMCR_SPEED100)
> -			speed = SPEED_100;
> -		else
> -			speed = SPEED_10;
> -	}
> -
>  	/*
>  	 * If mode switch happens from 10BT to 100BT, all DSP/AFE
>  	 * registers are set to default values. So any AFE/DSP
>  	 * registers have to be re-initialized in this case.
>  	 */
> -	if ((phydev->speed == SPEED_10) && (speed == SPEED_100)) {
> +	if (phydev->state == PHY_RUNNING && phydev->speed == SPEED_100) {
>  		int ret = rockchip_integrated_phy_analog_init(phydev);
> +
>  		if (ret)
>  			phydev_err(phydev, "rockchip_integrated_phy_analog_init err: %d.\n",
>  				   ret);
> 


-- 
Florian

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Florian Fainelli]

On 3/3/19 10:58 AM, Heiner Kallweit wrote:
> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Therefore let's change
> the behavior and call this callback only in case of an actual
> state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is neglectable and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

While you are it, we should probably audit all drivers making use of
PHYLIB and see whether the logic to compare the previous link with the
current link parameters is still necessary, and if it is, we should
consider moving that to the core PHYLIB.

> ---
>  drivers/net/phy/at803x.c   | 26 +++++++++-----------------
>  drivers/net/phy/phy.c      |  8 ++++----
>  drivers/net/phy/rockchip.c | 31 ++-----------------------------
>  3 files changed, 15 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index f3e96191e..f315ab468 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -324,8 +324,6 @@ static int at803x_config_intr(struct phy_device *phydev)
>  
>  static void at803x_link_change_notify(struct phy_device *phydev)
>  {
> -	struct at803x_priv *priv = phydev->priv;
> -
>  	/*
>  	 * Conduct a hardware reset for AT8030 every time a link loss is
>  	 * signalled. This is necessary to circumvent a hardware bug that
> @@ -333,25 +331,19 @@ static void at803x_link_change_notify(struct phy_device *phydev)
>  	 * in the FIFO. In such cases, the FIFO enters an error mode it
>  	 * cannot recover from by software.
>  	 */
> -	if (phydev->state == PHY_NOLINK) {
> -		if (phydev->mdio.reset && !priv->phy_reset) {
> -			struct at803x_context context;
> +	if (phydev->state == PHY_NOLINK && phydev->mdio.reset) {
> +		struct at803x_context context;
>  
> -			at803x_context_save(phydev, &context);
> +		at803x_context_save(phydev, &context);
>  
> -			phy_device_reset(phydev, 1);
> -			msleep(1);
> -			phy_device_reset(phydev, 0);
> -			msleep(1);
> +		phy_device_reset(phydev, 1);
> +		msleep(1);
> +		phy_device_reset(phydev, 0);
> +		msleep(1);
>  
> -			at803x_context_restore(phydev, &context);
> +		at803x_context_restore(phydev, &context);
>  
> -			phydev_dbg(phydev, "%s(): phy was reset\n",
> -				   __func__);
> -			priv->phy_reset = true;
> -		}
> -	} else {
> -		priv->phy_reset = false;
> +		phydev_dbg(phydev, "%s(): phy was reset\n", __func__);
>  	}
>  }
>  
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 3745220c5..5938c5acf 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -891,9 +891,6 @@ void phy_state_machine(struct work_struct *work)
>  
>  	old_state = phydev->state;
>  
> -	if (phydev->drv && phydev->drv->link_change_notify)
> -		phydev->drv->link_change_notify(phydev);
> -
>  	switch (phydev->state) {
>  	case PHY_DOWN:
>  	case PHY_READY:
> @@ -940,10 +937,13 @@ void phy_state_machine(struct work_struct *work)
>  	if (err < 0)
>  		phy_error(phydev);
>  
> -	if (old_state != phydev->state)
> +	if (old_state != phydev->state) {
>  		phydev_dbg(phydev, "PHY state change %s -> %s\n",
>  			   phy_state_to_str(old_state),
>  			   phy_state_to_str(phydev->state));
> +		if (phydev->drv && phydev->drv->link_change_notify)
> +			phydev->drv->link_change_notify(phydev);
> +	}
>  
>  	/* Only re-schedule a PHY state machine change if we are polling the
>  	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
> diff --git a/drivers/net/phy/rockchip.c b/drivers/net/phy/rockchip.c
> index 95abf7072..9053b1d01 100644
> --- a/drivers/net/phy/rockchip.c
> +++ b/drivers/net/phy/rockchip.c
> @@ -104,41 +104,14 @@ static int rockchip_integrated_phy_config_init(struct phy_device *phydev)
>  
>  static void rockchip_link_change_notify(struct phy_device *phydev)
>  {
> -	int speed = SPEED_10;
> -
> -	if (phydev->autoneg == AUTONEG_ENABLE) {
> -		int reg = phy_read(phydev, MII_SPECIAL_CONTROL_STATUS);
> -
> -		if (reg < 0) {
> -			phydev_err(phydev, "phy_read err: %d.\n", reg);
> -			return;
> -		}
> -
> -		if (reg & MII_SPEED_100)
> -			speed = SPEED_100;
> -		else if (reg & MII_SPEED_10)
> -			speed = SPEED_10;
> -	} else {
> -		int bmcr = phy_read(phydev, MII_BMCR);
> -
> -		if (bmcr < 0) {
> -			phydev_err(phydev, "phy_read err: %d.\n", bmcr);
> -			return;
> -		}
> -
> -		if (bmcr & BMCR_SPEED100)
> -			speed = SPEED_100;
> -		else
> -			speed = SPEED_10;
> -	}
> -
>  	/*
>  	 * If mode switch happens from 10BT to 100BT, all DSP/AFE
>  	 * registers are set to default values. So any AFE/DSP
>  	 * registers have to be re-initialized in this case.
>  	 */
> -	if ((phydev->speed == SPEED_10) && (speed == SPEED_100)) {
> +	if (phydev->state == PHY_RUNNING && phydev->speed == SPEED_100) {
>  		int ret = rockchip_integrated_phy_analog_init(phydev);
> +
>  		if (ret)
>  			phydev_err(phydev, "rockchip_integrated_phy_analog_init err: %d.\n",
>  				   ret);
> 


-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Russell King - ARM Linux admin]

On Tue, Mar 12, 2019 at 06:50:59PM +0100, Daniel Mack wrote:
> On 12/3/2019 1:27 PM, Heiko Stuebner wrote:
> > Hi Andrew, Heiner,
> > 
> > Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
> >> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
> >>> From: Heiner Kallweit <hkallweit1@gmail.com>
> >>> Date: Sun, 3 Mar 2019 19:58:57 +0100
> >>>
> >>>> Currently the Phy driver's link_change_notify callback is called
> >>>> whenever the state machine is run (every second if polling), no matter
> >>>> whether the state changed or not. This isn't needed and may confuse
> >>>> users considering the name of the callback. Therefore let's change
> >>>> the behavior and call this callback only in case of an actual
> >>>> state change.
> >>>>
> >>>> This requires changes to the at803x and rockchip drivers.
> >>>> at803x can be simplified so that it reacts on a state change to
> >>>> PHY_NOLINK only.
> >>>> The rockchip driver can also be much simplified. We simply re-init
> >>>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> >>>> is 100Mbps. This causes very small overhead because we do this even
> >>>> if the speed was 100Mbps already. But this is neglectable and
> >>>> I think justified by the much simpler code.
> >>>>
> >>>> Changes are compile-tested only.
> >>>>
> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>>
> >>> Someone please review this.
> >>
> >> Hi David
> >>
> >> We should probably wait for a Tested-by: from Daniel Mack
> >> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
> >> equivalent.
> > 
> > We should probably add them to the list of recipients if we want
> > tests from them, which I've done now.
> > 
> > @David: patch in question that changes the Rockchip eth-phy is
> > https://patchwork.kernel.org/patch/10837217/
> > 
> > Sadly I don't have matching hardware to test this myself.
> 
> Thanks for considering me, but I don't have access to that hardware
> either right now.
> 
> The changes look straight forward to me however, so just go ahead an
> apply this. Once I get back to that setup, I'll report in case of a
> regression.

SolidRun Hummingboards and Cubox-i use AR8035 with the FEC driver, so
would be ideal to test - unfortunately I'm up to my eyeballs trying to
diagnose Armada 8040 comphy issues so can't test at the moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[Russell King - ARM Linux admin]

On Tue, Mar 12, 2019 at 06:50:59PM +0100, Daniel Mack wrote:
> On 12/3/2019 1:27 PM, Heiko Stuebner wrote:
> > Hi Andrew, Heiner,
> > 
> > Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
> >> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
> >>> From: Heiner Kallweit <hkallweit1@gmail.com>
> >>> Date: Sun, 3 Mar 2019 19:58:57 +0100
> >>>
> >>>> Currently the Phy driver's link_change_notify callback is called
> >>>> whenever the state machine is run (every second if polling), no matter
> >>>> whether the state changed or not. This isn't needed and may confuse
> >>>> users considering the name of the callback. Therefore let's change
> >>>> the behavior and call this callback only in case of an actual
> >>>> state change.
> >>>>
> >>>> This requires changes to the at803x and rockchip drivers.
> >>>> at803x can be simplified so that it reacts on a state change to
> >>>> PHY_NOLINK only.
> >>>> The rockchip driver can also be much simplified. We simply re-init
> >>>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> >>>> is 100Mbps. This causes very small overhead because we do this even
> >>>> if the speed was 100Mbps already. But this is neglectable and
> >>>> I think justified by the much simpler code.
> >>>>
> >>>> Changes are compile-tested only.
> >>>>
> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>>
> >>> Someone please review this.
> >>
> >> Hi David
> >>
> >> We should probably wait for a Tested-by: from Daniel Mack
> >> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
> >> equivalent.
> > 
> > We should probably add them to the list of recipients if we want
> > tests from them, which I've done now.
> > 
> > @David: patch in question that changes the Rockchip eth-phy is
> > https://patchwork.kernel.org/patch/10837217/
> > 
> > Sadly I don't have matching hardware to test this myself.
> 
> Thanks for considering me, but I don't have access to that hardware
> either right now.
> 
> The changes look straight forward to me however, so just go ahead an
> apply this. Once I get back to that setup, I'll report in case of a
> regression.

SolidRun Hummingboards and Cubox-i use AR8035 with the FEC driver, so
would be ideal to test - unfortunately I'm up to my eyeballs trying to
diagnose Armada 8040 comphy issues so can't test at the moment.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Wu]

在 2019/3/13 上午1:50, Daniel Mack 写道:
> On 12/3/2019 1:27 PM, Heiko Stuebner wrote:
>> Hi Andrew, Heiner,
>>
>> Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
>>> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>>> Date: Sun, 3 Mar 2019 19:58:57 +0100
>>>>
>>>>> Currently the Phy driver's link_change_notify callback is called
>>>>> whenever the state machine is run (every second if polling), no matter
>>>>> whether the state changed or not. This isn't needed and may confuse
>>>>> users considering the name of the callback. Therefore let's change
>>>>> the behavior and call this callback only in case of an actual
>>>>> state change.
>>>>>
>>>>> This requires changes to the at803x and rockchip drivers.
>>>>> at803x can be simplified so that it reacts on a state change to
>>>>> PHY_NOLINK only.
>>>>> The rockchip driver can also be much simplified. We simply re-init
>>>>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
>>>>> is 100Mbps. This causes very small overhead because we do this even
>>>>> if the speed was 100Mbps already. But this is neglectable and
>>>>> I think justified by the much simpler code.
>>>>>
>>>>> Changes are compile-tested only.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>
>>>> Someone please review this.
>>>
>>> Hi David
>>>
>>> We should probably wait for a Tested-by: from Daniel Mack
>>> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
>>> equivalent.
>>
>> We should probably add them to the list of recipients if we want
>> tests from them, which I've done now.
>>
>> @David: patch in question that changes the Rockchip eth-phy is
>> https://patchwork.kernel.org/patch/10837217/

I think i can find a hardware to test this.

>>
>> Sadly I don't have matching hardware to test this myself.
> 
> Thanks for considering me, but I don't have access to that hardware
> either right now.
> 
> The changes look straight forward to me however, so just go ahead an
> apply this. Once I get back to that setup, I'll report in case of a
> regression.
> 
> 
> Thanks,
> Dainel
> 
> 
> 

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Wu]

在 2019/3/13 上午1:50, Daniel Mack 写道:
> On 12/3/2019 1:27 PM, Heiko Stuebner wrote:
>> Hi Andrew, Heiner,
>>
>> Am Montag, 4. März 2019, 21:06:30 CET schrieb Andrew Lunn:
>>> On Mon, Mar 04, 2019 at 11:30:25AM -0800, David Miller wrote:
>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>>> Date: Sun, 3 Mar 2019 19:58:57 +0100
>>>>
>>>>> Currently the Phy driver's link_change_notify callback is called
>>>>> whenever the state machine is run (every second if polling), no matter
>>>>> whether the state changed or not. This isn't needed and may confuse
>>>>> users considering the name of the callback. Therefore let's change
>>>>> the behavior and call this callback only in case of an actual
>>>>> state change.
>>>>>
>>>>> This requires changes to the at803x and rockchip drivers.
>>>>> at803x can be simplified so that it reacts on a state change to
>>>>> PHY_NOLINK only.
>>>>> The rockchip driver can also be much simplified. We simply re-init
>>>>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
>>>>> is 100Mbps. This causes very small overhead because we do this even
>>>>> if the speed was 100Mbps already. But this is neglectable and
>>>>> I think justified by the much simpler code.
>>>>>
>>>>> Changes are compile-tested only.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>
>>>> Someone please review this.
>>>
>>> Hi David
>>>
>>> We should probably wait for a Tested-by: from Daniel Mack
>>> <zonque@gmail.com> and David Wu <david.wu@rock-chips.com>, or
>>> equivalent.
>>
>> We should probably add them to the list of recipients if we want
>> tests from them, which I've done now.
>>
>> @David: patch in question that changes the Rockchip eth-phy is
>> https://patchwork.kernel.org/patch/10837217/

I think i can find a hardware to test this.

>>
>> Sadly I don't have matching hardware to test this myself.
> 
> Thanks for considering me, but I don't have access to that hardware
> either right now.
> 
> The changes look straight forward to me however, so just go ahead an
> apply this. Once I get back to that setup, I'll report in case of a
> regression.
> 
> 
> Thanks,
> Dainel
> 
> 
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Wu]

Hi Heiner,

Today i apply this patch, I test it on the RK3328 which has the phy,
it looks good, and link state switching between 10M/100M is correct.

在 2019/3/21 上午1:50, David Miller 写道:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Tue, 19 Mar 2019 19:56:51 +0100
> 
>> Currently the Phy driver's link_change_notify callback is called
>> whenever the state machine is run (every second if polling), no matter
>> whether the state changed or not. This isn't needed and may confuse
>> users considering the name of the callback. Actually it contradicts
>> its kernel-doc description. Therefore let's change the behavior and
>> call this callback only in case of an actual state change.
>>
>> This requires changes to the at803x and rockchip drivers.
>> at803x can be simplified so that it reacts on a state change to
>> PHY_NOLINK only.
>> The rockchip driver can also be much simplified. We simply re-init
>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
>> is 100Mbps. This causes very small overhead because we do this even
>> if the speed was 100Mbps already. But this is negligible and
>> I think justified by the much simpler code.
>>
>> Changes are compile-tested only.
>>
>> A little bit problematic seems to be to find somebody with the
>> hardware to test the changes to the two PHY drivers. See also [0].
>> David may be able to test the Rockchip driver.
>>
>> [0] https://marc.info/?t=153782508800006&r=1&w=2
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> I'm just going to apply this, let's see what happens as I don't really see
> any value after all of this time of waiting for testing that may or may not
> happen.
> 
> We can always revert.
> 
> Thanks.
> 
> 
> 

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Wu]

Hi Heiner,

Today i apply this patch, I test it on the RK3328 which has the phy,
it looks good, and link state switching between 10M/100M is correct.

在 2019/3/21 上午1:50, David Miller 写道:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Tue, 19 Mar 2019 19:56:51 +0100
> 
>> Currently the Phy driver's link_change_notify callback is called
>> whenever the state machine is run (every second if polling), no matter
>> whether the state changed or not. This isn't needed and may confuse
>> users considering the name of the callback. Actually it contradicts
>> its kernel-doc description. Therefore let's change the behavior and
>> call this callback only in case of an actual state change.
>>
>> This requires changes to the at803x and rockchip drivers.
>> at803x can be simplified so that it reacts on a state change to
>> PHY_NOLINK only.
>> The rockchip driver can also be much simplified. We simply re-init
>> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
>> is 100Mbps. This causes very small overhead because we do this even
>> if the speed was 100Mbps already. But this is negligible and
>> I think justified by the much simpler code.
>>
>> Changes are compile-tested only.
>>
>> A little bit problematic seems to be to find somebody with the
>> hardware to test the changes to the two PHY drivers. See also [0].
>> David may be able to test the Rockchip driver.
>>
>> [0] https://marc.info/?t=153782508800006&r=1&w=2
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> I'm just going to apply this, let's see what happens as I don't really see
> any value after all of this time of waiting for testing that may or may not
> happen.
> 
> We can always revert.
> 
> Thanks.
> 
> 
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 19 Mar 2019 19:56:51 +0100

> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Actually it contradicts
> its kernel-doc description. Therefore let's change the behavior and
> call this callback only in case of an actual state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is negligible and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> A little bit problematic seems to be to find somebody with the
> hardware to test the changes to the two PHY drivers. See also [0].
> David may be able to test the Rockchip driver.
> 
> [0] https://marc.info/?t=153782508800006&r=1&w=2
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

I'm just going to apply this, let's see what happens as I don't really see
any value after all of this time of waiting for testing that may or may not
happen.

We can always revert.

Thanks.

Re: [PATCH net-next] net: phy: improve handling link_change_notify callback

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Tue, 19 Mar 2019 19:56:51 +0100

> Currently the Phy driver's link_change_notify callback is called
> whenever the state machine is run (every second if polling), no matter
> whether the state changed or not. This isn't needed and may confuse
> users considering the name of the callback. Actually it contradicts
> its kernel-doc description. Therefore let's change the behavior and
> call this callback only in case of an actual state change.
> 
> This requires changes to the at803x and rockchip drivers.
> at803x can be simplified so that it reacts on a state change to
> PHY_NOLINK only.
> The rockchip driver can also be much simplified. We simply re-init
> the AFE/DSP registers whenever we change to PHY_RUNNING and speed
> is 100Mbps. This causes very small overhead because we do this even
> if the speed was 100Mbps already. But this is negligible and
> I think justified by the much simpler code.
> 
> Changes are compile-tested only.
> 
> A little bit problematic seems to be to find somebody with the
> hardware to test the changes to the two PHY drivers. See also [0].
> David may be able to test the Rockchip driver.
> 
> [0] https://marc.info/?t=153782508800006&r=1&w=2
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

I'm just going to apply this, let's see what happens as I don't really see
any value after all of this time of waiting for testing that may or may not
happen.

We can always revert.

Thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next] net: phy: improve handling link_change_notify callback

[Heiner Kallweit]

Currently the Phy driver's link_change_notify callback is called
whenever the state machine is run (every second if polling), no matter
whether the state changed or not. This isn't needed and may confuse
users considering the name of the callback. Actually it contradicts
its kernel-doc description. Therefore let's change the behavior and
call this callback only in case of an actual state change.

This requires changes to the at803x and rockchip drivers.
at803x can be simplified so that it reacts on a state change to
PHY_NOLINK only.
The rockchip driver can also be much simplified. We simply re-init
the AFE/DSP registers whenever we change to PHY_RUNNING and speed
is 100Mbps. This causes very small overhead because we do this even
if the speed was 100Mbps already. But this is negligible and
I think justified by the much simpler code.

Changes are compile-tested only.

A little bit problematic seems to be to find somebody with the
hardware to test the changes to the two PHY drivers. See also [0].
David may be able to test the Rockchip driver.

[0] https://marc.info/?t=153782508800006&r=1&w=2

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/at803x.c   | 26 +++++++++-----------------
 drivers/net/phy/phy.c      |  8 ++++----
 drivers/net/phy/rockchip.c | 31 ++-----------------------------
 3 files changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f3e96191e..f315ab468 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -324,8 +324,6 @@ static int at803x_config_intr(struct phy_device *phydev)
 
 static void at803x_link_change_notify(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
-
 	/*
 	 * Conduct a hardware reset for AT8030 every time a link loss is
 	 * signalled. This is necessary to circumvent a hardware bug that
@@ -333,25 +331,19 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	 * in the FIFO. In such cases, the FIFO enters an error mode it
 	 * cannot recover from by software.
 	 */
-	if (phydev->state == PHY_NOLINK) {
-		if (phydev->mdio.reset && !priv->phy_reset) {
-			struct at803x_context context;
+	if (phydev->state == PHY_NOLINK && phydev->mdio.reset) {
+		struct at803x_context context;
 
-			at803x_context_save(phydev, &context);
+		at803x_context_save(phydev, &context);
 
-			phy_device_reset(phydev, 1);
-			msleep(1);
-			phy_device_reset(phydev, 0);
-			msleep(1);
+		phy_device_reset(phydev, 1);
+		msleep(1);
+		phy_device_reset(phydev, 0);
+		msleep(1);
 
-			at803x_context_restore(phydev, &context);
+		at803x_context_restore(phydev, &context);
 
-			phydev_dbg(phydev, "%s(): phy was reset\n",
-				   __func__);
-			priv->phy_reset = true;
-		}
-	} else {
-		priv->phy_reset = false;
+		phydev_dbg(phydev, "%s(): phy was reset\n", __func__);
 	}
 }
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3745220c5..5938c5acf 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -891,9 +891,6 @@ void phy_state_machine(struct work_struct *work)
 
 	old_state = phydev->state;
 
-	if (phydev->drv && phydev->drv->link_change_notify)
-		phydev->drv->link_change_notify(phydev);
-
 	switch (phydev->state) {
 	case PHY_DOWN:
 	case PHY_READY:
@@ -940,10 +937,13 @@ void phy_state_machine(struct work_struct *work)
 	if (err < 0)
 		phy_error(phydev);
 
-	if (old_state != phydev->state)
+	if (old_state != phydev->state) {
 		phydev_dbg(phydev, "PHY state change %s -> %s\n",
 			   phy_state_to_str(old_state),
 			   phy_state_to_str(phydev->state));
+		if (phydev->drv && phydev->drv->link_change_notify)
+			phydev->drv->link_change_notify(phydev);
+	}
 
 	/* Only re-schedule a PHY state machine change if we are polling the
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
diff --git a/drivers/net/phy/rockchip.c b/drivers/net/phy/rockchip.c
index 95abf7072..9053b1d01 100644
--- a/drivers/net/phy/rockchip.c
+++ b/drivers/net/phy/rockchip.c
@@ -104,41 +104,14 @@ static int rockchip_integrated_phy_config_init(struct phy_device *phydev)
 
 static void rockchip_link_change_notify(struct phy_device *phydev)
 {
-	int speed = SPEED_10;
-
-	if (phydev->autoneg == AUTONEG_ENABLE) {
-		int reg = phy_read(phydev, MII_SPECIAL_CONTROL_STATUS);
-
-		if (reg < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", reg);
-			return;
-		}
-
-		if (reg & MII_SPEED_100)
-			speed = SPEED_100;
-		else if (reg & MII_SPEED_10)
-			speed = SPEED_10;
-	} else {
-		int bmcr = phy_read(phydev, MII_BMCR);
-
-		if (bmcr < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", bmcr);
-			return;
-		}
-
-		if (bmcr & BMCR_SPEED100)
-			speed = SPEED_100;
-		else
-			speed = SPEED_10;
-	}
-
 	/*
 	 * If mode switch happens from 10BT to 100BT, all DSP/AFE
 	 * registers are set to default values. So any AFE/DSP
 	 * registers have to be re-initialized in this case.
 	 */
-	if ((phydev->speed == SPEED_10) && (speed == SPEED_100)) {
+	if (phydev->state == PHY_RUNNING && phydev->speed == SPEED_100) {
 		int ret = rockchip_integrated_phy_analog_init(phydev);
+
 		if (ret)
 			phydev_err(phydev, "rockchip_integrated_phy_analog_init err: %d.\n",
 				   ret);
-- 
2.21.0

[PATCH net-next] net: phy: improve handling link_change_notify callback

[Heiner Kallweit]

Currently the Phy driver's link_change_notify callback is called
whenever the state machine is run (every second if polling), no matter
whether the state changed or not. This isn't needed and may confuse
users considering the name of the callback. Actually it contradicts
its kernel-doc description. Therefore let's change the behavior and
call this callback only in case of an actual state change.

This requires changes to the at803x and rockchip drivers.
at803x can be simplified so that it reacts on a state change to
PHY_NOLINK only.
The rockchip driver can also be much simplified. We simply re-init
the AFE/DSP registers whenever we change to PHY_RUNNING and speed
is 100Mbps. This causes very small overhead because we do this even
if the speed was 100Mbps already. But this is negligible and
I think justified by the much simpler code.

Changes are compile-tested only.

A little bit problematic seems to be to find somebody with the
hardware to test the changes to the two PHY drivers. See also [0].
David may be able to test the Rockchip driver.

[0] https://marc.info/?t=153782508800006&r=1&w=2

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/at803x.c   | 26 +++++++++-----------------
 drivers/net/phy/phy.c      |  8 ++++----
 drivers/net/phy/rockchip.c | 31 ++-----------------------------
 3 files changed, 15 insertions(+), 50 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index f3e96191e..f315ab468 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -324,8 +324,6 @@ static int at803x_config_intr(struct phy_device *phydev)
 
 static void at803x_link_change_notify(struct phy_device *phydev)
 {
-	struct at803x_priv *priv = phydev->priv;
-
 	/*
 	 * Conduct a hardware reset for AT8030 every time a link loss is
 	 * signalled. This is necessary to circumvent a hardware bug that
@@ -333,25 +331,19 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	 * in the FIFO. In such cases, the FIFO enters an error mode it
 	 * cannot recover from by software.
 	 */
-	if (phydev->state == PHY_NOLINK) {
-		if (phydev->mdio.reset && !priv->phy_reset) {
-			struct at803x_context context;
+	if (phydev->state == PHY_NOLINK && phydev->mdio.reset) {
+		struct at803x_context context;
 
-			at803x_context_save(phydev, &context);
+		at803x_context_save(phydev, &context);
 
-			phy_device_reset(phydev, 1);
-			msleep(1);
-			phy_device_reset(phydev, 0);
-			msleep(1);
+		phy_device_reset(phydev, 1);
+		msleep(1);
+		phy_device_reset(phydev, 0);
+		msleep(1);
 
-			at803x_context_restore(phydev, &context);
+		at803x_context_restore(phydev, &context);
 
-			phydev_dbg(phydev, "%s(): phy was reset\n",
-				   __func__);
-			priv->phy_reset = true;
-		}
-	} else {
-		priv->phy_reset = false;
+		phydev_dbg(phydev, "%s(): phy was reset\n", __func__);
 	}
 }
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3745220c5..5938c5acf 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -891,9 +891,6 @@ void phy_state_machine(struct work_struct *work)
 
 	old_state = phydev->state;
 
-	if (phydev->drv && phydev->drv->link_change_notify)
-		phydev->drv->link_change_notify(phydev);
-
 	switch (phydev->state) {
 	case PHY_DOWN:
 	case PHY_READY:
@@ -940,10 +937,13 @@ void phy_state_machine(struct work_struct *work)
 	if (err < 0)
 		phy_error(phydev);
 
-	if (old_state != phydev->state)
+	if (old_state != phydev->state) {
 		phydev_dbg(phydev, "PHY state change %s -> %s\n",
 			   phy_state_to_str(old_state),
 			   phy_state_to_str(phydev->state));
+		if (phydev->drv && phydev->drv->link_change_notify)
+			phydev->drv->link_change_notify(phydev);
+	}
 
 	/* Only re-schedule a PHY state machine change if we are polling the
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
diff --git a/drivers/net/phy/rockchip.c b/drivers/net/phy/rockchip.c
index 95abf7072..9053b1d01 100644
--- a/drivers/net/phy/rockchip.c
+++ b/drivers/net/phy/rockchip.c
@@ -104,41 +104,14 @@ static int rockchip_integrated_phy_config_init(struct phy_device *phydev)
 
 static void rockchip_link_change_notify(struct phy_device *phydev)
 {
-	int speed = SPEED_10;
-
-	if (phydev->autoneg == AUTONEG_ENABLE) {
-		int reg = phy_read(phydev, MII_SPECIAL_CONTROL_STATUS);
-
-		if (reg < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", reg);
-			return;
-		}
-
-		if (reg & MII_SPEED_100)
-			speed = SPEED_100;
-		else if (reg & MII_SPEED_10)
-			speed = SPEED_10;
-	} else {
-		int bmcr = phy_read(phydev, MII_BMCR);
-
-		if (bmcr < 0) {
-			phydev_err(phydev, "phy_read err: %d.\n", bmcr);
-			return;
-		}
-
-		if (bmcr & BMCR_SPEED100)
-			speed = SPEED_100;
-		else
-			speed = SPEED_10;
-	}
-
 	/*
 	 * If mode switch happens from 10BT to 100BT, all DSP/AFE
 	 * registers are set to default values. So any AFE/DSP
 	 * registers have to be re-initialized in this case.
 	 */
-	if ((phydev->speed == SPEED_10) && (speed == SPEED_100)) {
+	if (phydev->state == PHY_RUNNING && phydev->speed == SPEED_100) {
 		int ret = rockchip_integrated_phy_analog_init(phydev);
+
 		if (ret)
 			phydev_err(phydev, "rockchip_integrated_phy_analog_init err: %d.\n",
 				   ret);
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel