Re: [PATCH - REPOST] asix.c - Add support for AX88772A devices -

[David Hollis <dhollis@davehollis.com>]

On Wed, 2009-01-14 at 16:53 +0000, Ben Hutchings wrote:

> >  #define AX88772_IPG0_DEFAULT           0x15
> > -#define AX88772_IPG1_DEFAULT           0x0c
> > -#define AX88772_IPG2_DEFAULT           0x12
> > +#define AX88772_IPG1_DEFAULT           0x16
> > +#define AX88772_IPG2_DEFAULT           0x1A
> 
> Is this change correct for the plain AX88772 as well?
> 

I had to ask that as I well.  I have tested on a standard AX88772 and
they do work.  I definitely didn't want those devices breaking since I
use them quite regularly as well.   Apparently these values help in
certain half-duplex situations.


> >  /* GPIO 0 .. 2 toggles */
> >  #define AX_GPIO_GPO0EN         0x01    /* GPIO0 Output enable */
> >  #define AX_GPIO_GPO_0          0x02    /* GPIO0 Output value */
> > @@ -891,7 +898,11 @@ static int ax88772_link_reset(struct usb
> >         if (ecmd.duplex != DUPLEX_FULL)
> >                 mode &= ~AX_MEDIUM_FD;
> >  
> > -       devdbg(dev, "ax88772_link_reset() speed: %d duplex: %d setting mode to 0x%04x", ecmd.speed, ecmd.duplex, mode);
> > +       devdbg(dev,
> > +       "ax88772_link_reset() speed: %d duplex: %d setting mode to 0x%04x",
> > +               ecmd.speed,
> > +               ecmd.duplex,
> > +               mode);
> 
> This is just a formatting change so it belongs in a separate patch.  And
> if you're going to change it, the format string should be indented
> consistently with the other arguments.
> 

Good catch.  I missed that line.


> >         asix_write_medium_mode(dev, mode);
> >  
> > @@ -1018,6 +1029,121 @@ out:
> >         return ret;
> >  }
> >  
> > +static int ax88772a_bind(struct usbnet *dev, struct usb_interface *intf)
> > +{
> > +       int ret;
> > +       u16 rx_ctl;
> > +       struct asix_data *data = (struct asix_data *)&dev->data;
> > +       u8 buf[ETH_ALEN];
> > +       u32 phyid;
> > +
> > +       data->eeprom_len = AX88772_EEPROM_LEN;
> > +
> > +       usbnet_get_endpoints(dev,intf);
> > +
> > +       /* Power up the embedded PHY */
> > +       if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL)) < 0)
> > +               goto out;
> 
> Please separate the assignment and the test.  I know the existing code
> combines them and you don't need to fix that, but new code should not do
> this.
> 

I can work on doing that.  I'd really prefer that the whole file be one
way or the other, just so it's all consistent.



> > +       /* Select the embedded PHY as the active PHY */
> > +       if ((ret = asix_write_cmd(dev, AX_CMD_SW_PHY_SELECT,
> > +                               AX_PHYSELECT_SSEN | AX_PHYSELECT_PSEL,
> > +                               0, 0, NULL)) < 0) {
> > +               dbg("Select PHY #1 failed: %d", ret);
> > +               goto out;
> > +       }
> > +
> > +       /* Reload the EEPROM and configure the GPIO pins */
> > +       if ((ret = asix_write_gpio(dev,
> > +               AX_GPIO_RSE | AX_GPIO_GPO_2 | AX_GPIO_GPO2EN, 5)) < 0)
> > +       goto out;
> 
> "goto out;" should be indented.
> 

Another good catch.  Thanks.



> > +       /* Set embedded PHY in power down state */
> > +       if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL | AX_SWRESET_IPPD)) < 0)
> > +               goto out;
> > +
> > +       msleep(10);
> > +
> > +       /* Set embedded PHY in power up state */
> > +       if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL)) < 0)
> > +               goto out;
> > +
> > +       msleep(60);
> > +
> > +       /* Set embedded PHY in reset state */
> > +       if ((ret = asix_sw_reset(dev, AX_SWRESET_CLEAR)) < 0)
> > +               goto out;
> > +
> > +       /* Set embedded PHY in operating state */
> > +       if ((ret = asix_sw_reset(dev, AX_SWRESET_IPRL)) < 0)
> > +               goto out;
> > +
> > +       /* Stop RX operation */
> > +       if ((ret = asix_write_rx_ctl(dev, 0)) < 0)
> > +               goto out;       
> > +
> > +       /* Get the MAC address */
> > +       if ((ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID,
> > +                               0, 0, ETH_ALEN, buf)) < 0) {
> > +               dbg("Failed to read MAC address: %d", ret);
> > +               goto out;
> > +       }
> > +       memcpy(dev->net->dev_addr, buf, ETH_ALEN);
> > +
> > +       /* Initialize MII structure */
> > +       dev->mii.dev = dev->net;
> > +       dev->mii.mdio_read = asix_mdio_read;
> > +       dev->mii.mdio_write = asix_mdio_write;
> > +       dev->mii.phy_id_mask = 0x1f;
> > +       dev->mii.reg_num_mask = 0x1f;
> > +       dev->net->do_ioctl = asix_ioctl;
> > +       dev->mii.phy_id = asix_get_phy_addr(dev);
> > +
> > +       phyid = asix_get_phyid(dev);
> > +       dbg("PHYID=0x%08x", phyid);
> > +
> > +       dev->net->set_multicast_list = asix_set_multicast;
> > +       dev->net->ethtool_ops = &ax88772_ethtool_ops;
> > +
> > +       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);
> > +       mii_nway_restart(&dev->mii);
> > +
> > +       if ((ret = asix_write_medium_mode(dev, AX88772_MEDIUM_DEFAULT)) < 0)
> > +               goto out;
> > +
> > +       if ((ret = asix_write_cmd(dev, AX_CMD_WRITE_IPG0,
> > +                               AX88772_IPG0_DEFAULT | AX88772_IPG1_DEFAULT,
> > +                               AX88772_IPG2_DEFAULT, 0, NULL)) < 0) {
> > +               dbg("Write IPG,IPG1,IPG2 failed: %d", ret);
> > +               goto out;
> > +       }
> > +
> > +       msleep(1);
> > +
> > +       /* Set RX_CTL to default values with 2k buffer, and enable cactus */
> > +       if ((ret = asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL)) < 0)
> > +               goto out;
> > +
> > +       rx_ctl = asix_read_rx_ctl(dev);
> > +       dbg("RX_CTL is 0x%04x after all initializations", rx_ctl);
> > +
> > +       rx_ctl = asix_read_medium_status(dev);
> > +       dbg("Medium Status is 0x%04x after all initializations", rx_ctl);
> > +
> > +       /* Asix framing packs multiple eth frames into a 2K usb bulk transfer */
> > +       if (dev->driver_info->flags & FLAG_FRAMING_AX) {
> > +               /* hard_mtu  is still the default - the device does not support
> > +                  jumbo eth frames */
> > +               dev->rx_urb_size = 2048;
> > +       }
> > +       return 0;
> > +
> > +out:
> > +       return ret;
> > +}
> > +
> >  static struct ethtool_ops ax88178_ethtool_ops = {
> >         .get_drvinfo            = asix_get_drvinfo,
> >         .get_link               = asix_get_link,
> > @@ -1339,6 +1465,17 @@ static const struct driver_info ax88772_
> >         .tx_fixup = asix_tx_fixup,
> >  };
> >  
> > +static const struct driver_info ax88772a_info = {
> > +       .description = "ASIX AX88772A USB 2.0 Ethernet",
> > +       .bind = ax88772a_bind,
> > +       .status = asix_status,
> > +       .link_reset = ax88772_link_reset,
> > +       .reset = ax88772_link_reset,
> > +       .flags = FLAG_ETHER | FLAG_FRAMING_AX,
> > +       .rx_fixup = asix_rx_fixup,
> > +       .tx_fixup = asix_tx_fixup,
> > +};
> > +
> >  static const struct driver_info ax88178_info = {
> >         .description = "ASIX AX88178 USB 2.0 Ethernet",
> >         .bind = ax88178_bind,
> > @@ -1451,6 +1588,10 @@ static const struct usb_device_id        produc
> >         // Cables-to-Go USB Ethernet Adapter
> >         USB_DEVICE(0x0b95, 0x772a),
> >         .driver_info = (unsigned long) &ax88772_info,
> > +}, {
> > +       // ASIX AX88772A
> > +       USB_DEVICE(0x0b95, 0x772a),
> > +       .driver_info = (unsigned long) &ax88772a_info,
> 
> This is matching the same id as the entry above, so how will it ever be
> used?  Do the bind operations distinguish them somehow?
> 


Ugh!  I didn't notice that either.  Unfortunately, I don't have this new
device to be able to test with so I have to rely on the vendors
contribution.  I would presume that the Cables-to-Go device needs to be
using ax88772a_info and in the current state, it isn't going to.  I'll
check with the contributor for an assist on that.


> Please also run checkpatch.pl and fix the errors it finds.
> 

Will do, thanks.


> Ben.
> 
> >  },
> >         { },            // END
> >  };
>