Re: [PATCH net-next v5 3/3] smsc95xx: add phylib support

["Måns Rullgård" <mans@mansr.com>]

Andre Edich <andre.edich@microchip.com> writes:

> Generally, each PHY has their own configuration and it can be done
> through an external PHY driver.  The smsc95xx driver uses only the
> hard-coded internal PHY configuration.
>
> This patch adds phylib support to probe external PHY drivers for
> configuring external PHYs.
>
> The MDI-X configuration for the internal PHYs moves from
> drivers/net/usb/smsc95xx.c to drivers/net/phy/smsc.c.
>
> Signed-off-by: Andre Edich <andre.edich@microchip.com>
> ---
>  drivers/net/phy/smsc.c     |  67 +++++++
>  drivers/net/usb/Kconfig    |   2 +
>  drivers/net/usb/smsc95xx.c | 399 +++++++++++++------------------------
>  3 files changed, 203 insertions(+), 265 deletions(-)
>
> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> index 74568ae16125..638e8c3d1f4a 100644
> --- a/drivers/net/phy/smsc.c
> +++ b/drivers/net/phy/smsc.c
> @@ -21,6 +21,17 @@
>  #include <linux/netdevice.h>
>  #include <linux/smscphy.h>
>
> +/* Vendor-specific PHY Definitions */
> +/* EDPD NLP / crossover time configuration */
> +#define PHY_EDPD_CONFIG			16
> +#define PHY_EDPD_CONFIG_EXT_CROSSOVER_	0x0001
> +
> +/* Control/Status Indication Register */
> +#define SPECIAL_CTRL_STS		27
> +#define SPECIAL_CTRL_STS_OVRRD_AMDIX_	0x8000
> +#define SPECIAL_CTRL_STS_AMDIX_ENABLE_	0x4000
> +#define SPECIAL_CTRL_STS_AMDIX_STATE_	0x2000
> +
>  struct smsc_hw_stat {
>  	const char *string;
>  	u8 reg;
> @@ -96,6 +107,54 @@ static int lan911x_config_init(struct phy_device *phydev)
>  	return smsc_phy_ack_interrupt(phydev);
>  }
>
> +static int lan87xx_config_aneg(struct phy_device *phydev)
> +{
> +	int rc;
> +	int val;
> +
> +	switch (phydev->mdix_ctrl) {
> +	case ETH_TP_MDI:
> +		val = SPECIAL_CTRL_STS_OVRRD_AMDIX_;
> +		break;
> +	case ETH_TP_MDI_X:
> +		val = SPECIAL_CTRL_STS_OVRRD_AMDIX_ |
> +			SPECIAL_CTRL_STS_AMDIX_STATE_;
> +		break;
> +	case ETH_TP_MDI_AUTO:
> +		val = SPECIAL_CTRL_STS_AMDIX_ENABLE_;
> +		break;
> +	default:
> +		return genphy_config_aneg(phydev);
> +	}
> +
> +	rc = phy_read(phydev, SPECIAL_CTRL_STS);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc &= ~(SPECIAL_CTRL_STS_OVRRD_AMDIX_ |
> +		SPECIAL_CTRL_STS_AMDIX_ENABLE_ |
> +		SPECIAL_CTRL_STS_AMDIX_STATE_);
> +	rc |= val;
> +	phy_write(phydev, SPECIAL_CTRL_STS, rc);
> +
> +	phydev->mdix = phydev->mdix_ctrl;
> +	return genphy_config_aneg(phydev);
> +}
> +
> +static int lan87xx_config_aneg_ext(struct phy_device *phydev)
> +{
> +	int rc;
> +
> +	/* Extend Manual AutoMDIX timer */
> +	rc = phy_read(phydev, PHY_EDPD_CONFIG);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc |= PHY_EDPD_CONFIG_EXT_CROSSOVER_;
> +	phy_write(phydev, PHY_EDPD_CONFIG, rc);
> +	return lan87xx_config_aneg(phydev);
> +}
> +
>  /*
>   * The LAN87xx suffers from rare absence of the ENERGYON-bit when Ethernet cable
>   * plugs in while LAN87xx is in Energy Detect Power-Down mode. This leads to
> @@ -250,6 +309,9 @@ static struct phy_driver smsc_phy_driver[] = {
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> +	/* This covers internal PHY (phy_id: 0x0007C0C3) for
> +	 * LAN9500 (PID: 0x9500), LAN9514 (PID: 0xec00), LAN9505 (PID: 0x9505)
> +	 */
>  	.phy_id		= 0x0007c0c0, /* OUI=0x00800f, Model#=0x0c */
>  	.phy_id_mask	= 0xfffffff0,
>  	.name		= "SMSC LAN8700",
> @@ -262,6 +324,7 @@ static struct phy_driver smsc_phy_driver[] = {
>  	.read_status	= lan87xx_read_status,
>  	.config_init	= smsc_phy_config_init,
>  	.soft_reset	= smsc_phy_reset,
> +	.config_aneg	= lan87xx_config_aneg,
>
>  	/* IRQ related */
>  	.ack_interrupt	= smsc_phy_ack_interrupt,
> @@ -293,6 +356,9 @@ static struct phy_driver smsc_phy_driver[] = {
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
>  }, {
> +	/* This covers internal PHY (phy_id: 0x0007C0F0) for
> +	 * LAN9500A (PID: 0x9E00), LAN9505A (PID: 0x9E01)
> +	 */
>  	.phy_id		= 0x0007c0f0, /* OUI=0x00800f, Model#=0x0f */
>  	.phy_id_mask	= 0xfffffff0,
>  	.name		= "SMSC LAN8710/LAN8720",
> @@ -306,6 +372,7 @@ static struct phy_driver smsc_phy_driver[] = {
>  	.read_status	= lan87xx_read_status,
>  	.config_init	= smsc_phy_config_init,
>  	.soft_reset	= smsc_phy_reset,
> +	.config_aneg	= lan87xx_config_aneg_ext,
>
>  	/* IRQ related */
>  	.ack_interrupt	= smsc_phy_ack_interrupt,

This change seems to be causing some trouble I'm seeing with a LAN8710A.
Specifically lan87xx_config_aneg_ext() writes to register 16 which is
not documented for LAN8710A (nor for LAN8720A).  The effect is somewhat
random.  Sometimes, the device drops to 10 Mbps while the kernel still
reports the link speed as 100 Mbps.  Other times, it doesn't work at
all.  Everything works if I change config_aneg to lan87xx_config_aneg,
like this:

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 10722fed666d..07c0a7e4a350 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -408,7 +408,7 @@ static struct phy_driver smsc_phy_driver[] = {
        .read_status    = lan87xx_read_status,
        .config_init    = smsc_phy_config_init,
        .soft_reset     = smsc_phy_reset,
-       .config_aneg    = lan87xx_config_aneg_ext,
+       .config_aneg    = lan87xx_config_aneg,
 
        /* IRQ related */
        .ack_interrupt  = smsc_phy_ack_interrupt,

The internal phy of the LAN9500A does have a register 16 with
documentation matching the usage in this patch.  Unfortunately, there
doesn't seem to be any way of distinguishing this from the LAN8710A
based on register values.  Anyone got any clever ideas?

-- 
Måns Rullgård