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

[Heiner Kallweit]

Am 13.11.2014 um 23:02 schrieb 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.
Changed it to the proper order in my tree.
>>
>>> +			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.
In addition:
ar8236_hw_init seems to be generic enough to be used with ar8216 as well.
If we replace the dummy hw_init for ar8216 with ar8236_hw_init then we should
have proper hw initialization for ar8216 too and I'm confident that to restrict
the patch to ar8237/ar8337 is ok.
However I don't have a device with AR8216 to verify.
>>
>> - Felix
>>
> Heiner
> 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel