[OpenWrt-Devel] Autoneg problems with ar8216 on kernel 3.14 (related to ticket 17800)

John Crispin blogic at openwrt.org
Wed Nov 19 08:46:55 EST 2014



On 19/11/2014 14:44, Weedy wrote:
> 
> On Oct 30, 2014 5:08 PM, "Heiner Kallweit" <hkallweit1 at gmail.com
> <mailto:hkallweit1 at gmail.com>> wrote:
>>
>> Am 30.10.2014 um 07:22 schrieb Heiner Kallweit:
>> > Am 30.10.2014 um 03:36 schrieb Florian Fainelli:
>> >> Le 28/10/2014 11:46, Heiner Kallweit a écrit :
>> >>> After a little more thinking about it and looking at the code I
> basically have two questions:
>> >>>
>> >>> 1.
>> >>> Is it actually needed to exclude certain phy's in
> ar8xxx_phy_config_aneg?
>> >>> (At least for AR8327 it doesn't get called with an addr != 0 anyway)
>> >>> Else we could remove ar8xxx_phy_config_aneg and directly register
> genphy_config_aneg as
>> >>> callback for autoneg configuration.
>> >>
>> >> Address 0 is special, since this is a MDIO broadcast address that
> will usually be handled by switches as "write to all front-panel ports".
>> >>
>> >>>
>> >>> 2.
>> >>> Call sequence with regard to ar8216 is:
>> >>> ar8216: ar8xxx_phy_probe
>> >>> phy: phy_attach_direct
>> >>> phy: phy_init_hw
>> >>> ar8216: ar8xxx_phy_config_init
>> >>> ar8216: ar8xxx_phy_config_aneg
>> >>>
>> >>> ar8216 driver handles AR8327/AR8337 different from the other
> supported switch types.
>> >>> ar8xxx_start incl. more detailed configuration is called from
> ar8xxx_phy_probe already.
>> >>> For the other switch types it's called from ar8xxx_phy_config_init.
>> >>>
>> >>> I wonder whether doing detailed configuration in the probe stage
> might be too early.
>> >>> phy_init_hw resets the switch anyway later.
>> >>> Doing the detailed setup in ar8xxx_phy_config_init seems to be
> more suited however
>> >>> there might be good reason why it's handled the way it is.
>> >>
>> >> I suppose that you could re-advertise auto-negotiation by calling
> genphy_config_advert() in the config_init routine.
>> >
>> > The actual problem is caused by BMCR_ANENABLE being cleared by the
> reset in phy_init_hw.
>> > As far as I can see genphy_config_advert doesn't bring back this flag.
>> > What does genphy_config_aneg mainly do?
>> >   - call genphy_config_advert
>> >   - check if BMCR_ANENABLE is set
>> >   - if it's not, call genphy_restart_aneg
>> > Therefore, to bring back BMCR_ANENABLE, we have to call
> genphy_config_aneg or genphy_restart_aneg.
>> > genphy_restart_aneg most likely is sufficient, however I don't see
> that genphy_config_aneg
>> > can do any harm if being called with addr == 0.
>> > At least for me it works perfectly fine to call genphy_config_aneg
> for all addr's.
>> >
>> > Rgds, Heiner
>> >
>> >>
>> >>>
>> >>> Rgds,
>> >>> Heiner
>> >>>
>> >>>
>> >>> Am 27.10.2014 um 22:00 schrieb Heiner Kallweit:
>> >>>> With two different TPLINK routers (both with a AR8327N switch) I
> faced the issue that with
>> >>>> kernel 3.14 certain switch ports used 10MBit/half only.
>> >>>> Under kernel 3.10 everything was fine and the same ports used
> 1000MBit/full.
>> >>>>
>> >>>> As the ar8216 driver is the same I had a look at the common phy
> code in drivers/net/phy.
>> >>>> In function phy_init_hw in phy_device.c kernel 3.14 resets the
> phy whilst 3.10 does not.
>> >>>>
>> >>>> The issue turned out to be that when resetting only flag
> BMCR_RESET is set, not BMCR_ANENABLE.
>> >>>> (In ar8216.c always both flags are set when the switch is reset)
>> >>>> Therefore autoneg was not enabled. Also later in the boot process
> it doesn't get enabled.
>> >>>> Adding BMCR_ANENABLE solved the problem and now also under 3.14
> all ports use 1000MBit/full.
>> >>>>
>> >>>> However I'm not sure whether it's a poper fix to add
> BMCR_ANENABLE in this generic phy function.
>> >>>> It might not be appropriate for other phy's.
>> >>>>
>> >>>> After resetting the switch later in the boot process
> ar8xxx_phy_config_aneg is called with
>> >>>> phydev->addr being 0.
>> >>>> In this case the function returns immediately. Otherwise it would
> call genphy_config_aneg.
>> >>>> Calling genphy_config_aneg would also solve the problem as it
> checks for BMCR_ANENABLE
>> >>>> being set and sets it if needed.
>> >>>> I don't know the reason why genphy_config_aneg isn't called in
> case of addr 0.
>> >>>> Or is this a typo and the check actually should be addr != 0 ?
>> >>>>
>> >>>> Rgds, Heiner
>> >>>>
>>
>> The following rudimentary patch works fine for me with kernel 3.14.18 on
>> TP-LINK TL-WDR4900 (mpc85xx with AR8327Nv4)
>> TP-LINK TL-WDR4300 ( ar71xx with AR8327Nv2)
>>
>> Apart from using genphy_config_aneg also for addr==0 I replaced
> msleep(1000)
>> with a polling function inspired by phy_poll_reset in phy_device.c
>> On AR8327 the reset actually takes less than 20ms. Sleeping for 1000ms
>> seems to be a waste of time.
>>
>> Little more work would be needed to make it a proper patch:
>> - move ar8xxx_phy_poll_reset more to the beginning so that it is defined
>>   before being used in any xxxx_hw_init function
>> - replace msleep(1000) also in the other xxxx_hw_init functions
>> - remove pr_info debug message or make it at least pr_dbg
>> - insert some comments
>> - use git format-patch output
>>
>> Rgds, Heiner
>>
>>
>> --- ar8216.c.orig       2014-10-30 21:50:19.999723156 +0100
>> +++ ar8216.c    2014-10-30 21:42:11.996481099 +0100
>> @@ -1591,6 +1591,29 @@
>>  #endif
>>
>>  static int
>> +ar8xxx_phy_poll_reset(struct mii_bus *bus, int num_phys)
>> +{
>> +       unsigned int sleep_msecs = 20;
>> +       int ret, elapsed, i;
>> +
>> +       for(elapsed = sleep_msecs; elapsed <= 600; elapsed +=
> sleep_msecs) {
>> +               msleep(sleep_msecs);
>> +               for (i = 0; i < num_phys; i++) {
>> +                       ret = mdiobus_read(bus, i, MII_BMCR);
>> +                       if (ret < 0) return ret;
>> +                       if (ret & BMCR_RESET) break;
>> +                       if (i == num_phys - 1) {
>> +                               usleep_range(1000, 2000);
>> +                               pr_info("ar8xxx_phy_poll_reset
> finished in %u ms\n",
>> +                                       elapsed);
>> +                               return 0;
>> +                       }
>> +               }
>> +       }
>> +       return -ETIMEDOUT;
>> +}
>> +
>> +static int
>>  ar8327_hw_init(struct ar8xxx_priv *priv)
>>  {
>>         struct mii_bus *bus;
>> @@ -1620,7 +1643,7 @@
>>                 mdiobus_write(bus, i, MII_BMCR, BMCR_RESET |
> BMCR_ANENABLE);
>>         }
>>
>> -       msleep(1000);
>> +       ar8xxx_phy_poll_reset(bus, AR8327_NUM_PHYS);
>>
>>         return 0;
>>  }
>> @@ -2840,15 +2863,6 @@
>>         return ret;
>>  }
>>
>> -static int
>> -ar8xxx_phy_config_aneg(struct phy_device *phydev)
>> -{
>> -       if (phydev->addr == 0)
>> -               return 0;
>> -
>> -       return genphy_config_aneg(phydev);
>> -}
>> -
>>  static const u32 ar8xxx_phy_ids[] = {
>>         0x004dd033,
>>         0x004dd034, /* AR8327 */
>> @@ -3020,7 +3034,7 @@
>>         .remove         = ar8xxx_phy_remove,
>>         .detach         = ar8xxx_phy_detach,
>>         .config_init    = ar8xxx_phy_config_init,
>> -       .config_aneg    = ar8xxx_phy_config_aneg,
>> +       .config_aneg    = genphy_config_aneg,
>>         .read_status    = ar8xxx_phy_read_status,
>>         .driver         = { .owner = THIS_MODULE },
>>  };
>> _______________________________________________
>> openwrt-devel mailing list
>> openwrt-devel at lists.openwrt.org <mailto:openwrt-devel at lists.openwrt.org>
>> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
> 
> I feel like this shouldn't die off in the depths of the ML
> 

we had a call about it today, this patch and the other ar821x patches
will be handled soon

> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
> 
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel



More information about the openwrt-devel mailing list