diff mbox series

[net-next,05/19] net: usb: aqc111: Introduce PHY access

Message ID 6b4837b970d7709bc2e11d89a7e21e5f10584e30.1538734658.git.igor.russkikh@aquantia.com (mailing list archive)
State Superseded
Headers show
Series Add support for Aquantia AQtion USB to 5/2.5GbE devices | expand

Commit Message

Igor Russkikh Oct. 5, 2018, 10:24 a.m. UTC 
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(+)

Comments

Andrew Lunn Oct. 5, 2018, 10:04 p.m. UTC | #1 
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, &reg16);
> +
> +	/* Power down ethernet PHY */
> +	if (aqc111_data->dpa) {
> +		reg8 = 0x00;
> +		aqc111_write_cmd_nopm(dev, AQ_PHY_POWER, 0,
> +				      0, 1, &reg8);
> +	} 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
Igor Russkikh Oct. 8, 2018, 9:09 a.m. UTC | #2 
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
Andrew Lunn Oct. 8, 2018, 12:17 p.m. UTC | #3 
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
Oliver Neukum Oct. 8, 2018, 1:52 p.m. UTC | #4 
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
Igor Russkikh Oct. 8, 2018, 2:07 p.m. UTC | #5 
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
Igor Russkikh Oct. 8, 2018, 2:10 p.m. UTC | #6 
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
Oliver Neukum Oct. 8, 2018, 2:24 p.m. UTC | #7 
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
Andrew Lunn Oct. 10, 2018, 12:58 a.m. UTC | #8 
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
Igor Russkikh Oct. 10, 2018, 7:54 a.m. UTC | #9 
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 mbox series

Patch

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, &reg16);
+
+	/* Power down ethernet PHY */
+	if (aqc111_data->dpa) {
+		reg8 = 0x00;
+		aqc111_write_cmd_nopm(dev, AQ_PHY_POWER, 0,
+				      0, 1, &reg8);
+	} 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, &reg8);
+		if (reg8 == 0x00) {
+			reg8 = 0x02;
+			aqc111_write_cmd(dev, AQ_PHY_POWER, 0, 0, 1, &reg8);
+			msleep(200);
+		}
+
+		aq_mdio_read_cmd(dev, AQ_GLB_STD_CTRL_REG, AQ_PHY_GLOBAL_ADDR,
+				 2, &reg16);
+		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, &reg16);
+		}
+	} 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, &reg8);
@@ -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, &reg16);
@@ -214,6 +296,17 @@  static int aqc111_stop(struct usbnet *dev)
 	aqc111_write_cmd(dev, AQ_ACCESS_MAC, SFR_RX_CTL,
 			 2, 2, &reg16);
 
+	/* 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, &reg16);
+	} 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;