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