| Message ID | 6b4837b970d7709bc2e11d89a7e21e5f10584e30.1538734658.git.igor.russkikh@aquantia.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add support for Aquantia AQtion USB to 5/2.5GbE devices | expand |
On Fri, Oct 05, 2018 at 10:24:53AM +0000, Igor Russkikh wrote: > From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com> > > Implement PHY power up/down sequences. > AQC111, depending on FW used, may has PHY being controlled either > directly (dpa = 1) or via vendor command interface (dpa = 0). Hi Igor dpa is not a very descriptive name. Once we figure out if phylib is going to be used, or even phylink, i suggest you rename this to something like aqc111_data->use_phylib. > @@ -172,6 +211,8 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf) > u8 reg8; > u16 reg16; > > + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; Having to do this cast all the time is quiet ugly. It seems like some other usb_net drivers use netdev_priv(). > + > /* Force bz */ g/> reg16 = SFR_PHYPWR_RSTCTL_BZ; > aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL, > @@ -179,12 +220,52 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf) > reg16 = 0; > aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL, > 2, 2, ®16); > + > + /* Power down ethernet PHY */ > + if (aqc111_data->dpa) { > + reg8 = 0x00; > + aqc111_write_cmd_nopm(dev, AQ_PHY_POWER, 0, > + 0, 1, ®8); > + } else { > + aqc111_data->phy_ops.low_power = 1; > + aqc111_data->phy_ops.phy_power = 0; > + aqc111_write_cmd_nopm(dev, AQ_PHY_OPS, 0, 0, > + 4, &aqc111_data->phy_ops); > + } > + > + kfree(aqc111_data); > } > > +struct aqc111_phy_options { > + union { > + struct { > + u8 adv_100M: 1; > + u8 adv_1G: 1; > + u8 adv_2G5: 1; > + u8 adv_5G: 1; > + u8 rsvd1: 4; > + }; > + u8 advertising; > + }; > + union { > + struct { > + u8 eee_100M: 1; > + u8 eee_1G: 1; > + u8 eee_2G5: 1; > + u8 eee_5G: 1; > + u8 rsvd2: 4; > + }; > + u8 eee; > + }; > + union { > + struct { > + u8 pause: 1; > + u8 asym_pause: 1; > + u8 low_power: 1; > + u8 phy_power: 1; > + u8 wol: 1; > + u8 downshift: 1; > + u8 rsvd4: 2; > + }; > + u8 phy_ctrl1; > + }; > + union { > + struct { > + u8 dsh_ret_cnt: 4; > + u8 magic_packet:1; > + u8 rsvd5: 3; The indentation looks wrong here. > + }; > + u8 phy_ctrl2; > + }; > +}; > + > +struct aqc111_data { > + struct { > + u8 major; > + u8 minor; > + u8 rev; > + } fw_ver; > + u8 dpa; /*direct PHY access*/ > + struct aqc111_phy_options phy_ops; > +} __packed; Why pack this? Do you send it to the firmware? Andrew
Hi Andrew, >> >> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; > > Having to do this cast all the time is quiet ugly. It seems like some > other usb_net drivers use netdev_priv(). As I see most of usb usbnet based devices use the same theme with accessing private data via dev->data. netdev_priv() is used to store struct usbnet itself. >> + u8 dpa; /*direct PHY access*/ >> + struct aqc111_phy_options phy_ops; >> +} __packed; > > Why pack this? Do you send it to the firmware? Agreed, no. We have to pack phy_ops and wol_config only. Regards, Igor
On Mon, Oct 08, 2018 at 09:09:54AM +0000, Igor Russkikh wrote: > Hi Andrew, > > >> > >> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; > > > > Having to do this cast all the time is quiet ugly. It seems like some > > other usb_net drivers use netdev_priv(). > > As I see most of usb usbnet based devices use the same theme with accessing > private data via dev->data. It is just ugly. It would of been better if dev->data[] was a void pointer. This is the first usbnet driver i've reviewed, so i don't know the history behind this. I wonder if adding a void *priv would be accepted? Andrew
On Fr, 2018-10-05 at 10:24 +0000, Igor Russkikh wrote: > From: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com> > > Implement PHY power up/down sequences. > AQC111, depending on FW used, may has PHY being controlled either > directly (dpa = 1) or via vendor command interface (dpa = 0). > Drivers supports both themes. > We determine this from firmware versioning agreement. > > Signed-off-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com> > Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com> > --- > drivers/net/usb/aqc111.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++ > drivers/net/usb/aqc111.h | 70 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 163 insertions(+) > > diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c > index 22bb259d71fb..30219bb6ddfd 100644 > --- a/drivers/net/usb/aqc111.c > +++ b/drivers/net/usb/aqc111.c > @@ -138,15 +138,44 @@ static int aqc111_write_cmd(struct usbnet *dev, u8 cmd, u16 value, > return __aqc111_write_cmd(dev, cmd, value, index, size, data, 0); > } > > +static int aq_mdio_read_cmd(struct usbnet *dev, u16 value, u16 index, > + u16 size, void *data) > +{ > + return aqc111_read_cmd(dev, AQ_PHY_CMD, value, index, size, data); > +} > + > +static int aq_mdio_write_cmd(struct usbnet *dev, u16 value, u16 index, > + u16 size, void *data) > +{ > + return aqc111_write_cmd(dev, AQ_PHY_CMD, value, index, size, data); > +} > + > static const struct net_device_ops aqc111_netdev_ops = { > .ndo_open = usbnet_open, > .ndo_stop = usbnet_stop, > }; > > +static void aqc111_read_fw_version(struct usbnet *dev, > + struct aqc111_data *aqc111_data) > +{ > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR, > + 1, 1, &aqc111_data->fw_ver.major); > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR, > + 1, 1, &aqc111_data->fw_ver.minor); > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV, > + 1, 1, &aqc111_data->fw_ver.rev); Why read the stuff you don't need? > + > + if (aqc111_data->fw_ver.major & 0x80) > + aqc111_data->fw_ver.major &= ~0x80; > + else > + aqc111_data->dpa = 1; > +} > + > static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf) > { > int ret; > struct usb_device *udev = interface_to_usbdev(intf); > + struct aqc111_data *aqc111_data; > > /* Check if vendor configuration */ > if (udev->actconfig->desc.bConfigurationValue != 1) { > @@ -162,8 +191,18 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf) > return ret; > } > > + aqc111_data = kzalloc(sizeof(*aqc111_data), GFP_KERNEL); > + if (!aqc111_data) > + return -ENOMEM; > + > + /* store aqc111_data pointer in device data field */ > + dev->data[0] = (unsigned long)aqc111_data; > + memset(aqc111_data, 0, sizeof(*aqc111_data)); Either kzalloc() or a memset. Not both. > + > dev->net->netdev_ops = &aqc111_netdev_ops; > > + aqc111_read_fw_version(dev, aqc111_data); > + > return 0; > } Regards Oliver
Hi Andrew, >>>> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; >>> >>> Having to do this cast all the time is quiet ugly. It seems like some >>> other usb_net drivers use netdev_priv(). >> >> As I see most of usb usbnet based devices use the same theme with accessing >> private data via dev->data. > > It is just ugly. It would of been better if dev->data[] was a void > pointer. This is the first usbnet driver i've reviewed, so i don't > know the history behind this. I wonder if adding a void *priv would be > accepted? I can't comment on history of this, but... net-next/drivers/net/usb$ grep "dev->data" * | wc -l 173 Regards, Igor
Hi Oliver, >> + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR, >> + 1, 1, &aqc111_data->fw_ver.major); >> + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR, >> + 1, 1, &aqc111_data->fw_ver.minor); >> + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV, >> + 1, 1, &aqc111_data->fw_ver.rev); > > Why read the stuff you don't need? fw_ver is used below to determine phy access mode. fw_ver.rev is not used in this exact patch, thats true, but it gets reported in later patches in the set. Regards, Igor
On Mo, 2018-10-08 at 17:10 +0300, Igor Russkikh wrote: > Hi Oliver, > > > > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR, > > > + 1, 1, &aqc111_data->fw_ver.major); > > > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR, > > > + 1, 1, &aqc111_data->fw_ver.minor); > > > + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV, > > > + 1, 1, &aqc111_data->fw_ver.rev); > > > > Why read the stuff you don't need? > > fw_ver is used below to determine phy access mode. > > fw_ver.rev is not used in this exact patch, thats true, > but it gets reported in later patches in the set. Hi, OK that makes sense. Regards Oliver
On Mon, Oct 08, 2018 at 09:09:54AM +0000, Igor Russkikh wrote: > Hi Andrew, > > >> > >> + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; > > > > Having to do this cast all the time is quiet ugly. It seems like some > > other usb_net drivers use netdev_priv(). > > As I see most of usb usbnet based devices use the same theme with accessing > private data via dev->data. Hi Igor I just discovered driver_priv. https://elixir.bootlin.com/linux/v4.19-rc7/source/include/linux/usb/usbnet.h#L33 It would be good to use that, to avoid the casts. Andrew
On 10.10.2018 03:58, Andrew Lunn wrote: > > I just discovered driver_priv. > > https://elixir.bootlin.com/linux/v4.19-rc7/source/include/linux/usb/usbnet.h#L33 > > It would be good to use that, to avoid the casts. Looks good to me, thanks. Regards, Igor
diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c index 22bb259d71fb..30219bb6ddfd 100644 --- a/drivers/net/usb/aqc111.c +++ b/drivers/net/usb/aqc111.c @@ -138,15 +138,44 @@ static int aqc111_write_cmd(struct usbnet *dev, u8 cmd, u16 value, return __aqc111_write_cmd(dev, cmd, value, index, size, data, 0); } +static int aq_mdio_read_cmd(struct usbnet *dev, u16 value, u16 index, + u16 size, void *data) +{ + return aqc111_read_cmd(dev, AQ_PHY_CMD, value, index, size, data); +} + +static int aq_mdio_write_cmd(struct usbnet *dev, u16 value, u16 index, + u16 size, void *data) +{ + return aqc111_write_cmd(dev, AQ_PHY_CMD, value, index, size, data); +} + static const struct net_device_ops aqc111_netdev_ops = { .ndo_open = usbnet_open, .ndo_stop = usbnet_stop, }; +static void aqc111_read_fw_version(struct usbnet *dev, + struct aqc111_data *aqc111_data) +{ + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MAJOR, + 1, 1, &aqc111_data->fw_ver.major); + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_MINOR, + 1, 1, &aqc111_data->fw_ver.minor); + aqc111_read_cmd(dev, AQ_ACCESS_MAC, AQ_FW_VER_REV, + 1, 1, &aqc111_data->fw_ver.rev); + + if (aqc111_data->fw_ver.major & 0x80) + aqc111_data->fw_ver.major &= ~0x80; + else + aqc111_data->dpa = 1; +} + static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf) { int ret; struct usb_device *udev = interface_to_usbdev(intf); + struct aqc111_data *aqc111_data; /* Check if vendor configuration */ if (udev->actconfig->desc.bConfigurationValue != 1) { @@ -162,8 +191,18 @@ static int aqc111_bind(struct usbnet *dev, struct usb_interface *intf) return ret; } + aqc111_data = kzalloc(sizeof(*aqc111_data), GFP_KERNEL); + if (!aqc111_data) + return -ENOMEM; + + /* store aqc111_data pointer in device data field */ + dev->data[0] = (unsigned long)aqc111_data; + memset(aqc111_data, 0, sizeof(*aqc111_data)); + dev->net->netdev_ops = &aqc111_netdev_ops; + aqc111_read_fw_version(dev, aqc111_data); + return 0; } @@ -172,6 +211,8 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf) u8 reg8; u16 reg16; + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; + /* Force bz */ reg16 = SFR_PHYPWR_RSTCTL_BZ; aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL, @@ -179,12 +220,52 @@ static void aqc111_unbind(struct usbnet *dev, struct usb_interface *intf) reg16 = 0; aqc111_write_cmd_nopm(dev, AQ_ACCESS_MAC, SFR_PHYPWR_RSTCTL, 2, 2, ®16); + + /* Power down ethernet PHY */ + if (aqc111_data->dpa) { + reg8 = 0x00; + aqc111_write_cmd_nopm(dev, AQ_PHY_POWER, 0, + 0, 1, ®8); + } else { + aqc111_data->phy_ops.low_power = 1; + aqc111_data->phy_ops.phy_power = 0; + aqc111_write_cmd_nopm(dev, AQ_PHY_OPS, 0, 0, + 4, &aqc111_data->phy_ops); + } + + kfree(aqc111_data); } static int aqc111_reset(struct usbnet *dev) { u8 reg8 = 0; u16 reg16 = 0; + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; + + /* Power up ethernet PHY */ + aqc111_data->phy_ops.phy_ctrl1 = 0; + aqc111_data->phy_ops.phy_ctrl2 = 0; + + aqc111_data->phy_ops.phy_power = 1; + if (aqc111_data->dpa) { + aqc111_read_cmd(dev, AQ_PHY_POWER, 0, 0, 1, ®8); + if (reg8 == 0x00) { + reg8 = 0x02; + aqc111_write_cmd(dev, AQ_PHY_POWER, 0, 0, 1, ®8); + msleep(200); + } + + aq_mdio_read_cmd(dev, AQ_GLB_STD_CTRL_REG, AQ_PHY_GLOBAL_ADDR, + 2, ®16); + if (reg16 & AQ_PHY_LOW_POWER_MODE) { + reg16 &= ~AQ_PHY_LOW_POWER_MODE; + aq_mdio_write_cmd(dev, AQ_GLB_STD_CTRL_REG, + AQ_PHY_GLOBAL_ADDR, 2, ®16); + } + } else { + aqc111_write_cmd(dev, AQ_PHY_OPS, 0, 0, + 4, &aqc111_data->phy_ops); + } reg8 = 0xFF; aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_BM_INT_MASK, 1, 1, ®8); @@ -204,6 +285,7 @@ static int aqc111_reset(struct usbnet *dev) static int aqc111_stop(struct usbnet *dev) { u16 reg16 = 0; + struct aqc111_data *aqc111_data = (struct aqc111_data *)dev->data[0]; aqc111_read_cmd(dev, AQ_ACCESS_MAC, SFR_MEDIUM_STATUS_MODE, 2, 2, ®16); @@ -214,6 +296,17 @@ static int aqc111_stop(struct usbnet *dev) aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_RX_CTL, 2, 2, ®16); + /* Put PHY to low power*/ + if (aqc111_data->dpa) { + reg16 = AQ_PHY_LOW_POWER_MODE; + aq_mdio_write_cmd(dev, AQ_GLB_STD_CTRL_REG, AQ_PHY_GLOBAL_ADDR, + 2, ®16); + } else { + aqc111_data->phy_ops.low_power = 1; + aqc111_write_cmd(dev, AQ_PHY_OPS, 0, 0, + 4, &aqc111_data->phy_ops); + } + return 0; } diff --git a/drivers/net/usb/aqc111.h b/drivers/net/usb/aqc111.h index 6b34202fa22b..8738d2c4ae90 100644 --- a/drivers/net/usb/aqc111.h +++ b/drivers/net/usb/aqc111.h @@ -12,6 +12,17 @@ #define AQ_ACCESS_MAC 0x01 #define AQ_PHY_POWER 0x31 +#define AQ_PHY_CMD 0x32 +#define AQ_PHY_OPS 0x61 + +#define AQC111_PHY_ID 0x00 +#define AQ_PHY_ADDR(mmd) ((AQC111_PHY_ID << 8) | mmd) + +#define AQ_PHY_GLOBAL_MMD 0x1E +#define AQ_PHY_GLOBAL_ADDR AQ_PHY_ADDR(AQ_PHY_GLOBAL_MMD) + +#define AQ_GLB_STD_CTRL_REG 0x0000 + #define AQ_PHY_LOW_POWER_MODE 0x0800 #define AQ_USB_PHY_SET_TIMEOUT 10000 #define AQ_USB_SET_TIMEOUT 4000 @@ -102,6 +113,65 @@ #define SFR_BULK_OUT_FLUSH_EN 0x01 #define SFR_BULK_OUT_EFF_EN 0x02 +#define AQ_FW_VER_MAJOR 0xDA +#define AQ_FW_VER_MINOR 0xDB +#define AQ_FW_VER_REV 0xDC + +/******************************************************************************/ + +struct aqc111_phy_options { + union { + struct { + u8 adv_100M: 1; + u8 adv_1G: 1; + u8 adv_2G5: 1; + u8 adv_5G: 1; + u8 rsvd1: 4; + }; + u8 advertising; + }; + union { + struct { + u8 eee_100M: 1; + u8 eee_1G: 1; + u8 eee_2G5: 1; + u8 eee_5G: 1; + u8 rsvd2: 4; + }; + u8 eee; + }; + union { + struct { + u8 pause: 1; + u8 asym_pause: 1; + u8 low_power: 1; + u8 phy_power: 1; + u8 wol: 1; + u8 downshift: 1; + u8 rsvd4: 2; + }; + u8 phy_ctrl1; + }; + union { + struct { + u8 dsh_ret_cnt: 4; + u8 magic_packet:1; + u8 rsvd5: 3; + }; + u8 phy_ctrl2; + }; +}; + +struct aqc111_data { + struct { + u8 major; + u8 minor; + u8 rev; + } fw_ver; + u8 dpa; /*direct PHY access*/ + struct aqc111_phy_options phy_ops; +} __packed; + static struct { unsigned char ctrl; unsigned char timer_l;