On Oct 30, 2014 5:08 PM, "Heiner Kallweit" <[email protected]> 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 > [email protected] > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
I feel like this shouldn't die off in the depths of the ML
_______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
