[OpenWrt-Devel] [PATCH] ar8216: fix issue with ANEG being disabled with kernel >=3.14

[Heiner Kallweit]

Am 13.11.2014 um 22:18 schrieb Felix Fietkau:
> On 2014-11-13 22:00, Heiner Kallweit wrote:
>> See also ticket 17800
>> With kernel>=3.14 autonegotiation is disabled at least for AR8327 based
>> switches. Reason is that with 3.14 an additional phy reset was
>> introduced in phy_init_hw in drivers/net/phy/phy_device.c
>> This reset clears BMCR_ANENABLE.
>> After the reset phy_init_hw calls the driver's config_init callback
>> which however for ar8327/8337 does nothing.
>> Fix the issue by extending ar8xxx_phy_config_init to check for
>> BMCR_ANENABLE being set in case of ar8327/ar8337.
>> If needed set the flag and restart autonegotiation.
>>
>> For kernel>=3.16 the phy reset in phy_init_hw can be overwritten by
>> a soft_reset callback provided by the phy driver.
>> ar8216 driver takes care of resetting the switch properly for all
>> supported switch types anyway, therefore provide a dummy soft_reset
>> callback to disable the unneeded additional phy reset.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1 at gmail.com>
>> ---
>>  .../linux/generic/files/drivers/net/phy/ar8216.c   | 34 ++++++++++++++++++++--
>>  1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/linux/generic/files/drivers/net/phy/ar8216.c b/target/linux/generic/files/drivers/net/phy/ar8216.c
>> index 4410fbb..03de384 100644
>> --- a/target/linux/generic/files/drivers/net/phy/ar8216.c
>> +++ b/target/linux/generic/files/drivers/net/phy/ar8216.c
>> @@ -36,6 +36,7 @@
>>  #include <linux/of_device.h>
>>  #include <linux/leds.h>
>>  #include <linux/gpio.h>
>> +#include <linux/version.h>
>>  
>>  #include "ar8216.h"
>>  
>> @@ -2765,8 +2766,24 @@ ar8xxx_phy_config_init(struct phy_device *phydev)
>>  	if (WARN_ON(!priv))
>>  		return -ENODEV;
>>  
>> -	if (chip_is_ar8327(priv) || chip_is_ar8337(priv))
>> -		return 0;
>> +	if (chip_is_ar8327(priv) || chip_is_ar8337(priv)) {
>> +		if (AUTONEG_ENABLE != phydev->autoneg)
> Why the yoda condition?
Actually I reused parts of the code from genphy_config_aneg which includes
this yoda condition. Not sure how it survived the code review when it was
added to the kernel code.
I also wondered why the kernel code includes a yoda condition.
> 
>> +			return 0;
>> +		/*
>> +		 * BMCR_ANENABLE might have been cleared
>> +		 * by phy_init_hw in certain kernel versions
>> +		 * therefore check for it
>> +		 */
>> +		ret = phy_read(phydev, MII_BMCR);
>> +		if (ret < 0)
>> +			return ret;
>> +		if (ret & BMCR_ANENABLE)
>> +			return 0;
>> +
>> +		dev_info(&phydev->dev, "ANEG disabled, re-enabling ..\n");
>> +		ret |= BMCR_ANENABLE | BMCR_ANRESTART;
>> +		return phy_write(phydev, MII_BMCR, ret);
> Are you sure that this is only needed on AR8327/AR8337?
Only for AR8327/AR8337 the switch is initialized at probe time already.
For AR8236/AR8316 ar8xxx_config_init calls the respective hw_init callback
which resets the switch incl. setting BMCR_ANENABLE.
So for these chips we should be fine.
Just for AR8216 hw_init does nothing. For AR8216 BMCR_ANENABLE is never set
in the driver code as far as I can see.
Not sure whether it's intentional to do no hw initialization at all for AR8216
and completely relying on the status we find it in.
Having said that I'm not sure regarding AR8216.
> 
> - Felix
> 
Heiner
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel