[PATCH 0/3] net: phy: dp83867: sync dp83867_phy_reset

[PATCH 0/3] net: phy: dp83867: sync dp83867_phy_reset

[Stefan Kerkmann]

I ran into the same problem as Roland with this Phy driver and missed
the fix that landed in 095cd32961aab64cfe72941ce43d99876852e12b ("net:
phy: dp83867: reset PHY on probe"). I'm still submitting this series as
a refactoring of the fix - with the goal of being closer to the upstream
Linux implementation.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
Stefan Kerkmann (3):
      net: phy: allow PHY drivers to implement their own software reset
      net: phy: document core PHY structures
      net: phy: dp83867: sync dp83867_phy_rest

 drivers/net/phy/dp83867.c | 28 ++++++++++++++++++++++------
 drivers/net/phy/phy.c     | 16 ++++++++++++++--
 include/linux/phy.h       | 25 ++++++++++++++++---------
 3 files changed, 52 insertions(+), 17 deletions(-)
---
base-commit: 8ad1b68cf755873866945f77e2e635a2c47e4c5e
change-id: 20240523-feature-dp83867-soft-reset-d9dc632017c4

Best regards,
-- 
Stefan Kerkmann <s.kerkmann@pengutronix.de>

[PATCH 1/3] net: phy: allow PHY drivers to implement their own software reset

[Stefan Kerkmann]

This is a port of linux commit 9df81dd7583d14862d0cfb673a941b261f3b2112
("net: phy: allow PHY drivers to implement their own software reset")
which implements the ability for phy drivers to implement the own
non-standard software reset sequence.

A side effect is that fixups will now applied always even if
.config_init is undefined. This shouldn't be possible to happen though,
because phy_driver_register will populate the member with
genphy_config_init in that case, so the member should never be NULL.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 drivers/net/phy/phy.c | 16 ++++++++++++++--
 include/linux/phy.h   |  5 +++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index abd78b2c80..a83f183302 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1108,14 +1108,26 @@ int phy_init_hw(struct phy_device *phydev)
 	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
 	int ret;
 
-	if (!phydrv || !phydrv->config_init)
+	if (!phydrv)
 		return 0;
 
+	if (phydrv->soft_reset) {
+		ret = phydrv->soft_reset(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = phy_scan_fixups(phydev);
 	if (ret < 0)
 		return ret;
 
-	return phydrv->config_init(phydev);
+	if (phydrv->config_init) {
+		ret = phydrv->config_init(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 
 static struct phy_driver genphy_driver = {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7da4f94e0e..a6b96a5984 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -275,6 +275,11 @@ struct phy_driver {
 	const void *driver_data;
 	bool is_phy;
 
+	/**
+	 * @soft_reset: Called to issue a PHY software reset
+	 */
+	int (*soft_reset)(struct phy_device *phydev);
+
 	/*
 	 * Called to initialize the PHY,
 	 * including after a reset

-- 
2.39.2

Re: [PATCH 1/3] net: phy: allow PHY drivers to implement their own software reset

[Ahmad Fatoum]

On 23.05.24 16:40, Stefan Kerkmann wrote:
> This is a port of linux commit 9df81dd7583d14862d0cfb673a941b261f3b2112
> ("net: phy: allow PHY drivers to implement their own software reset")
> which implements the ability for phy drivers to implement the own
> non-standard software reset sequence.
> 
> A side effect is that fixups will now applied always even if
> .config_init is undefined. This shouldn't be possible to happen though,
> because phy_driver_register will populate the member with
> genphy_config_init in that case, so the member should never be NULL.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/net/phy/phy.c | 16 ++++++++++++++--
>  include/linux/phy.h   |  5 +++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index abd78b2c80..a83f183302 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1108,14 +1108,26 @@ int phy_init_hw(struct phy_device *phydev)
>  	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
>  	int ret;
>  
> -	if (!phydrv || !phydrv->config_init)
> +	if (!phydrv)
>  		return 0;
>  
> +	if (phydrv->soft_reset) {
> +		ret = phydrv->soft_reset(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	ret = phy_scan_fixups(phydev);
>  	if (ret < 0)
>  		return ret;
>  
> -	return phydrv->config_init(phydev);
> +	if (phydrv->config_init) {
> +		ret = phydrv->config_init(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static struct phy_driver genphy_driver = {
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 7da4f94e0e..a6b96a5984 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -275,6 +275,11 @@ struct phy_driver {
>  	const void *driver_data;
>  	bool is_phy;
>  
> +	/**
> +	 * @soft_reset: Called to issue a PHY software reset
> +	 */
> +	int (*soft_reset)(struct phy_device *phydev);
> +
>  	/*
>  	 * Called to initialize the PHY,
>  	 * including after a reset
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 2/3] net: phy: document core PHY structures

[Stefan Kerkmann]

This is a port of linux commit 4069a572d423b73919ae40a500dfc4b10f8a6f32
("net: phy: Document core PHY structures"), that copies the Doxygen
comments for the PHY structure where applicable.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 include/linux/phy.h | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index a6b96a5984..ef25dec033 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -280,36 +280,38 @@ struct phy_driver {
 	 */
 	int (*soft_reset)(struct phy_device *phydev);
 
-	/*
-	 * Called to initialize the PHY,
+	/**
+	 * @config_init: Called to initialize the PHY,
 	 * including after a reset
 	 */
 	int (*config_init)(struct phy_device *phydev);
 
-	/*
-	 * Called during discovery.  Used to set
+	/**
+	 * @probe: Called during discovery.  Used to set
 	 * up device-specific structures, if any
 	 */
 	int (*probe)(struct phy_device *phydev);
 
-	/*
-	 * Configures the advertisement and resets
+	/**
+	 * @config_aneg: Configures the advertisement and resets
 	 * autonegotiation if phydev->autoneg is on,
 	 * forces the speed to the current settings in phydev
 	 * if phydev->autoneg is off
 	 */
 	int (*config_aneg)(struct phy_device *phydev);
 
-	/* Determines the auto negotiation result */
+	/** @aneg_done: Determines the auto negotiation result */
 	int (*aneg_done)(struct phy_device *phydev);
 
-	/* Determines the negotiated speed and duplex */
+	/** @read_status: Determines the negotiated speed and duplex */
 	int (*read_status)(struct phy_device *phydev);
 
-	/* Clears up any memory if needed */
+	/** @remove: Clears up any memory if needed */
 	void (*remove)(struct phy_device *phydev);
 
+	/** @read_page: Return the current PHY register page number */
 	int (*read_page)(struct phy_device *phydev);
+	/** @write_page: Set the current PHY register page number */
 	int (*write_page)(struct phy_device *phydev, int page);
 
 	struct driver	 drv;

-- 
2.39.2

Re: [PATCH 2/3] net: phy: document core PHY structures

[Ahmad Fatoum]

On 23.05.24 16:40, Stefan Kerkmann wrote:
> This is a port of linux commit 4069a572d423b73919ae40a500dfc4b10f8a6f32
> ("net: phy: Document core PHY structures"), that copies the Doxygen
> comments for the PHY structure where applicable.
> 
> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  include/linux/phy.h | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index a6b96a5984..ef25dec033 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -280,36 +280,38 @@ struct phy_driver {
>  	 */
>  	int (*soft_reset)(struct phy_device *phydev);
>  
> -	/*
> -	 * Called to initialize the PHY,
> +	/**
> +	 * @config_init: Called to initialize the PHY,
>  	 * including after a reset
>  	 */
>  	int (*config_init)(struct phy_device *phydev);
>  
> -	/*
> -	 * Called during discovery.  Used to set
> +	/**
> +	 * @probe: Called during discovery.  Used to set
>  	 * up device-specific structures, if any
>  	 */
>  	int (*probe)(struct phy_device *phydev);
>  
> -	/*
> -	 * Configures the advertisement and resets
> +	/**
> +	 * @config_aneg: Configures the advertisement and resets
>  	 * autonegotiation if phydev->autoneg is on,
>  	 * forces the speed to the current settings in phydev
>  	 * if phydev->autoneg is off
>  	 */
>  	int (*config_aneg)(struct phy_device *phydev);
>  
> -	/* Determines the auto negotiation result */
> +	/** @aneg_done: Determines the auto negotiation result */
>  	int (*aneg_done)(struct phy_device *phydev);
>  
> -	/* Determines the negotiated speed and duplex */
> +	/** @read_status: Determines the negotiated speed and duplex */
>  	int (*read_status)(struct phy_device *phydev);
>  
> -	/* Clears up any memory if needed */
> +	/** @remove: Clears up any memory if needed */
>  	void (*remove)(struct phy_device *phydev);
>  
> +	/** @read_page: Return the current PHY register page number */
>  	int (*read_page)(struct phy_device *phydev);
> +	/** @write_page: Set the current PHY register page number */
>  	int (*write_page)(struct phy_device *phydev, int page);
>  
>  	struct driver	 drv;
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/3] net: phy: dp83867: sync dp83867_phy_rest

[Stefan Kerkmann]

This is a port of the `dp83867_phy_reset` function at the state of linux
commit a129b41fe0a8b4da828c46b10f5244ca07a3fec3 ("Revert "net: phy:
dp83867: perform soft reset and retain established link"). Which
performs a reset before starting the initial configuration.

It is a refactoring of commit 095cd32961aab64cfe72941ce43d99876852e12b
("net: phy: dp83867: reset PHY on probe") which already ported parts of
the function, but diverted from the upstream implementation.

Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
---
 drivers/net/phy/dp83867.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index aefc651489..3c9a8e6355 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -362,8 +362,6 @@ static int dp83867_of_init(struct phy_device *phydev)
 	return 0;
 }
 
-static int dp83867_phy_reset(struct phy_device *phydev); /* see below */
-
 static int dp83867_probe(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867;
@@ -372,8 +370,6 @@ static int dp83867_probe(struct phy_device *phydev)
 
 	phydev->priv = dp83867;
 
-	dp83867_phy_reset(phydev);
-
 	return dp83867_of_init(phydev);
 }
 
@@ -571,14 +567,33 @@ static int dp83867_phy_reset(struct phy_device *phydev)
 {
 	int err;
 
+	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
+	if (err < 0)
+		return err;
+
+	udelay(20);
+
+	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
+			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
+	if (err < 0)
+		return err;
+
+	/* Configure the DSP Feedforward Equalizer Configuration register to
+	 * improve short cable (< 1 meter) performance. This will not affect
+	 * long cable performance.
+	 */
+	err = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG,
+			    0x0e81);
+	if (err < 0)
+		return err;
+
 	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESTART);
 	if (err < 0)
 		return err;
 
 	udelay(20);
 
-	return phy_modify(phydev, MII_DP83867_PHYCTRL,
-			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
+	return 0;
 }
 
 static struct phy_driver dp83867_driver[] = {
@@ -590,6 +605,7 @@ static struct phy_driver dp83867_driver[] = {
 
 		.probe          = dp83867_probe,
 		.config_init	= dp83867_config_init,
+		.soft_reset	= dp83867_phy_reset,
 
 		.read_status	= dp83867_read_status,
 	},

-- 
2.39.2

Re: [PATCH 3/3] net: phy: dp83867: sync dp83867_phy_rest

[Ahmad Fatoum]

On 23.05.24 16:40, Stefan Kerkmann wrote:
> This is a port of the `dp83867_phy_reset` function at the state of linux
> commit a129b41fe0a8b4da828c46b10f5244ca07a3fec3 ("Revert "net: phy:
> dp83867: perform soft reset and retain established link"). Which
> performs a reset before starting the initial configuration.
> 
> It is a refactoring of commit 095cd32961aab64cfe72941ce43d99876852e12b
> ("net: phy: dp83867: reset PHY on probe") which already ported parts of
> the function, but diverted from the upstream implementation.

Even if it doesn't matter for your original issue, the DP83867_DSP_FFE_CFG
configuration you import looks like a good thing to have.

> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/net/phy/dp83867.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index aefc651489..3c9a8e6355 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -362,8 +362,6 @@ static int dp83867_of_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> -static int dp83867_phy_reset(struct phy_device *phydev); /* see below */
> -
>  static int dp83867_probe(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867;
> @@ -372,8 +370,6 @@ static int dp83867_probe(struct phy_device *phydev)
>  
>  	phydev->priv = dp83867;
>  
> -	dp83867_phy_reset(phydev);
> -
>  	return dp83867_of_init(phydev);
>  }
>  
> @@ -571,14 +567,33 @@ static int dp83867_phy_reset(struct phy_device *phydev)
>  {
>  	int err;
>  
> +	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESET);
> +	if (err < 0)
> +		return err;
> +
> +	udelay(20);
> +
> +	err = phy_modify(phydev, MII_DP83867_PHYCTRL,
> +			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
> +	if (err < 0)
> +		return err;
> +
> +	/* Configure the DSP Feedforward Equalizer Configuration register to
> +	 * improve short cable (< 1 meter) performance. This will not affect
> +	 * long cable performance.
> +	 */
> +	err = phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_DSP_FFE_CFG,
> +			    0x0e81);
> +	if (err < 0)
> +		return err;
> +
>  	err = phy_write(phydev, DP83867_CTRL, DP83867_SW_RESTART);
>  	if (err < 0)
>  		return err;
>  
>  	udelay(20);
>  
> -	return phy_modify(phydev, MII_DP83867_PHYCTRL,
> -			 DP83867_PHYCR_FORCE_LINK_GOOD, 0);
> +	return 0;
>  }
>  
>  static struct phy_driver dp83867_driver[] = {
> @@ -590,6 +605,7 @@ static struct phy_driver dp83867_driver[] = {
>  
>  		.probe          = dp83867_probe,
>  		.config_init	= dp83867_config_init,
> +		.soft_reset	= dp83867_phy_reset,
>  
>  		.read_status	= dp83867_read_status,
>  	},
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/3] net: phy: dp83867: sync dp83867_phy_reset

[Sascha Hauer]

On Thu, 23 May 2024 16:40:27 +0200, Stefan Kerkmann wrote:
> I ran into the same problem as Roland with this Phy driver and missed
> the fix that landed in 095cd32961aab64cfe72941ce43d99876852e12b ("net:
> phy: dp83867: reset PHY on probe"). I'm still submitting this series as
> a refactoring of the fix - with the goal of being closer to the upstream
> Linux implementation.
> 
> 
> [...]

Applied, thanks!

[1/3] net: phy: allow PHY drivers to implement their own software reset
      https://git.pengutronix.de/cgit/barebox/commit/?id=bca02a0f549f (link may not be stable)
[2/3] net: phy: document core PHY structures
      https://git.pengutronix.de/cgit/barebox/commit/?id=7fb2f20dc854 (link may not be stable)
[3/3] net: phy: dp83867: sync dp83867_phy_rest
      https://git.pengutronix.de/cgit/barebox/commit/?id=adde7a0344ac (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>