On 19/11/2014 14:44, Weedy wrote: > > On Oct 30, 2014 5:08 PM, "Heiner Kallweit" <hkallwe...@gmail.com > <mailto:hkallwe...@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@lists.openwrt.org <mailto:openwrt-devel@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@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel > _______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel