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:b...@decadent.org.uk] Sent: Wednesday, November 12, 2014 1:23 AM To: Charles Keepax Cc: Stam, Michel [FINT]; Riku Voipio; da...@davemloft.net; linux-...@vger.kernel.org; net...@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-...@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
