| Message ID | 20141104200914.GN23178@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2014-11-04 at 20:09 +0000, Charles Keepax wrote: > On Tue, Nov 04, 2014 at 11:23:06AM +0100, Stam, Michel [FINT] wrote: > > Hello Riku, > > > > >Fixing a bug (ethtool support) must not cause breakage elsewhere (in > > this case on arndale). This is now a regression of functionality from > > 3.17. > > > > > >I think it would better to revert the change now and with less hurry > > introduce a ethtool fix that doesn't break arndale. > > > > I don't fully agree here; > > I would like to point out that this commit is a revert itself. Fixing > > the armdale will then cause breakage in other implementations, such as > > ours. Blankly reverting breaks other peoples' implementations. > > > > The PHY reset is the thing that breaks ethtool support, so any fix that > > appeases all would have to take existing PHY state into account. [...] > --- a/drivers/net/usb/asix_devices.c > +++ b/drivers/net/usb/asix_devices.c > @@ -299,6 +299,7 @@ static int ax88772_reset(struct usbnet *dev) > { > struct asix_data *data = (struct asix_data *)&dev->data; > int ret, embd_phy; > + int reg; > u16 rx_ctl; > > ret = asix_write_gpio(dev, > @@ -359,8 +360,10 @@ static int ax88772_reset(struct usbnet *dev) > msleep(150); > > asix_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET); > - asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, > - ADVERTISE_ALL | ADVERTISE_CSMA); > + reg = asix_mdio_read(dev->net, dev->mii.phy_id, MII_ADVERTISE); > + if (!reg) > + reg = ADVERTISE_ALL | ADVERTISE_CSMA; > + asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, reg); [...] Why is there no sleep after setting the RESET bit? Doesn't that make the following register writes unreliable? Ben.
Hello Ben, Regarding the code snippet; Good question, The original code didn't do this either, which is why I left it as it is. It could cause undesirable behaviour, agreed. After a quick driver examination: I do see that asix_set_sw_mii and asix_set_hw_mii are called prior to the actual write (asix_mdio_write), it may be that this takes care of ensuring data is written to the chip, as asix_write_cmd waits for usbnet_write_cmd to send (and acknowledge) a USB CONTROL MSG. A lock to the phy is held during this time. If I recall my USB knowledge, control messages are acknowledged, which would ensure data is written to the chip. Whether the ASIX requires further delay I would not know. I would have to dive deeper into the timings of the ASIX chip and the driver behaviour to see if that would be the case. Kind regards, Michel Stam -----Original Message----- From: Ben Hutchings [mailto:ben@decadent.org.uk] Sent: Wednesday, November 12, 2014 1:23 AM To: Charles Keepax Cc: Stam, Michel [FINT]; Riku Voipio; davem@davemloft.net; linux-usb@vger.kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-soc@vger.kernel.org Subject: Re: "asix: Don't reset PHY on if_up for ASIX 88772" breaks net on arndale platform On Tue, 2014-11-04 at 20:09 +0000, Charles Keepax wrote: > On Tue, Nov 04, 2014 at 11:23:06AM +0100, Stam, Michel [FINT] wrote: > > Hello Riku, > > > > >Fixing a bug (ethtool support) must not cause breakage elsewhere > > >(in > > this case on arndale). This is now a regression of functionality > > from 3.17. > > > > > >I think it would better to revert the change now and with less > > >hurry > > introduce a ethtool fix that doesn't break arndale. > > > > I don't fully agree here; > > I would like to point out that this commit is a revert itself. > > Fixing the armdale will then cause breakage in other > > implementations, such as ours. Blankly reverting breaks other peoples' implementations. > > > > The PHY reset is the thing that breaks ethtool support, so any fix > > that appeases all would have to take existing PHY state into account. [...] > --- a/drivers/net/usb/asix_devices.c > +++ b/drivers/net/usb/asix_devices.c > @@ -299,6 +299,7 @@ static int ax88772_reset(struct usbnet *dev) { > struct asix_data *data = (struct asix_data *)&dev->data; > int ret, embd_phy; > + int reg; > u16 rx_ctl; > > ret = asix_write_gpio(dev, > @@ -359,8 +360,10 @@ static int ax88772_reset(struct usbnet *dev) > msleep(150); > > asix_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET); > - asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, > - ADVERTISE_ALL | ADVERTISE_CSMA); > + reg = asix_mdio_read(dev->net, dev->mii.phy_id, MII_ADVERTISE); > + if (!reg) > + reg = ADVERTISE_ALL | ADVERTISE_CSMA; > + asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, > + reg); [...] Why is there no sleep after setting the RESET bit? Doesn't that make the following register writes unreliable? Ben. -- Ben Hutchings Experience is directly proportional to the value of equipment destroyed. - Carolyn Scheppner
Please do not top-post. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/net/usb/asix_devices.c +++ b/drivers/net/usb/asix_devices.c @@ -299,6 +299,7 @@ static int ax88772_reset(struct usbnet *dev) { struct asix_data *data = (struct asix_data *)&dev->data; int ret, embd_phy; + int reg; u16 rx_ctl; ret = asix_write_gpio(dev, @@ -359,8 +360,10 @@ static int ax88772_reset(struct usbnet *dev) msleep(150); asix_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET); - asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, - ADVERTISE_ALL | ADVERTISE_CSMA); + reg = asix_mdio_read(dev->net, dev->mii.phy_id, MII_ADVERTISE); + if (!reg) + reg = ADVERTISE_ALL | ADVERTISE_CSMA; + asix_mdio_write(dev->net, dev->mii.phy_id, MII_ADVERTISE, reg); mii_nway_restart(&dev->mii); ret = asix_write_medium_mode(dev, AX88772_MEDIUM_DEFAULT);