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

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

Heiner Kallweit <hkallweit1@gmail.com> writes:

> On 28.03.2021 21:59, Måns Rullgård wrote:
>> 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?
>> 
> After reading register PHY_EDPD_CONFIG you could check whether the
> read value is plausible. On the PHY's not supporting this register,
> what is the read value? 0x00 or 0xff? And is this value plausible
> for PHY's supporting this register?

On the LAN8710A I have here, it reads as 0x40.  That bit is "reserved"
without a specified value on the LAN9500A.  On LAN8740A (which also has
a different ID value), it controls Energy Efficient Ethernet

> Currently the PHY driver doesn't check the revision number (last four
> bits of PHY ID). Maybe they differ.

Even if they are different today (I don't seem to have a LAN9500A to
check), nothing guarantees that this will remain the case with future
versions of the chips.

-- 
Måns Rullgård