[U-Boot] [PATCH V3 1/3] usb: eth: fix Makefile

[U-Boot] [PATCH V3 1/3] usb: eth: fix Makefile

[Rene Griessl]

fix issue as discussed with Marek Vasut

Signed-off-by: Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
---
 drivers/usb/eth/Makefile | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
index 94551c4..e6ae9f1 100644
--- a/drivers/usb/eth/Makefile
+++ b/drivers/usb/eth/Makefile
@@ -5,8 +5,6 @@
 
 # new USB host ethernet layer dependencies
 obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
-ifdef CONFIG_USB_ETHER_ASIX
-obj-y += asix.o
-endif
+obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
 obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
 obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
-- 
1.9.1

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[Rene Griessl]

changes in v3:
	-added all compatible devices from linux driver
	-fixed issues from review

changes in v2: 
        -added usb_ether.h to change list 
        -added 2nd patch to enable driver for arndale board 

Signed-off-by: Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
---
 drivers/usb/eth/Makefile    |   1 +
 drivers/usb/eth/asix88179.c | 659 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/eth/usb_ether.c |   7 +
 include/usb_ether.h         |   6 +
 4 files changed, 673 insertions(+)
 create mode 100644 drivers/usb/eth/asix88179.c

diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
index e6ae9f1..c92d2b0 100644
--- a/drivers/usb/eth/Makefile
+++ b/drivers/usb/eth/Makefile
@@ -6,5 +6,6 @@
 # new USB host ethernet layer dependencies
 obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
 obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
+obj-$(CONFIG_USB_ETHER_ASIX88179) += asix88179.o
 obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
 obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
diff --git a/drivers/usb/eth/asix88179.c b/drivers/usb/eth/asix88179.c
new file mode 100644
index 0000000..2079056
--- /dev/null
+++ b/drivers/usb/eth/asix88179.c
@@ -0,0 +1,659 @@
+/*
+ * Copyright (c) 2014 Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
+ * based on the U-Boot Asix driver as well as information
+ * from the Linux AX88179_178a driver
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+
+#include <common.h>
+#include <usb.h>
+#include <net.h>
+#include <linux/mii.h>
+#include "usb_ether.h"
+#include <malloc.h>
+
+
+/* ASIX AX88179 based USB 3.0 Ethernet Devices */
+#define AX88179_PHY_ID				0x03
+#define AX_EEPROM_LEN				0x100
+#define AX88179_EEPROM_MAGIC			0x17900b95
+#define AX_MCAST_FLTSIZE			8
+#define AX_MAX_MCAST				64
+#define AX_INT_PPLS_LINK			(1 << 16)
+#define AX_RXHDR_L4_TYPE_MASK			0x1c
+#define AX_RXHDR_L4_TYPE_UDP			4
+#define AX_RXHDR_L4_TYPE_TCP			16
+#define AX_RXHDR_L3CSUM_ERR			2
+#define AX_RXHDR_L4CSUM_ERR			1
+#define AX_RXHDR_CRC_ERR			(1 << 29)
+#define AX_RXHDR_DROP_ERR			(1 << 31)
+#define AX_ACCESS_MAC				0x01
+#define AX_ACCESS_PHY				0x02
+#define AX_ACCESS_EEPROM			0x04
+#define AX_ACCESS_EFUS				0x05
+#define AX_PAUSE_WATERLVL_HIGH			0x54
+#define AX_PAUSE_WATERLVL_LOW			0x55
+
+#define PHYSICAL_LINK_STATUS			0x02
+	#define	AX_USB_SS		0x04
+	#define	AX_USB_HS		0x02
+
+#define GENERAL_STATUS				0x03
+/* Check AX88179 version. UA1:Bit2 = 0,  UA2:Bit2 = 1 */
+	#define	AX_SECLD		0x04
+
+#define AX_SROM_ADDR				0x07
+#define AX_SROM_CMD				0x0a
+	#define EEP_RD			0x04
+	#define EEP_BUSY		0x10
+
+#define AX_SROM_DATA_LOW			0x08
+#define AX_SROM_DATA_HIGH			0x09
+
+#define AX_RX_CTL				0x0b
+	#define AX_RX_CTL_DROPCRCERR	0x0100
+	#define AX_RX_CTL_IPE		0x0200
+	#define AX_RX_CTL_START		0x0080
+	#define AX_RX_CTL_AP		0x0020
+	#define AX_RX_CTL_AM		0x0010
+	#define AX_RX_CTL_AB		0x0008
+	#define AX_RX_CTL_AMALL		0x0002
+	#define AX_RX_CTL_PRO		0x0001
+	#define AX_RX_CTL_STOP		0x0000
+
+#define AX_NODE_ID				0x10
+#define AX_MULFLTARY				0x16
+
+#define AX_MEDIUM_STATUS_MODE			0x22
+	#define AX_MEDIUM_GIGAMODE	0x01
+	#define AX_MEDIUM_FULL_DUPLEX	0x02
+	#define AX_MEDIUM_EN_125MHZ	0x08
+	#define AX_MEDIUM_RXFLOW_CTRLEN	0x10
+	#define AX_MEDIUM_TXFLOW_CTRLEN	0x20
+	#define AX_MEDIUM_RECEIVE_EN	0x100
+	#define AX_MEDIUM_PS		0x200
+	#define AX_MEDIUM_JUMBO_EN	0x8040
+
+#define AX_MONITOR_MOD				0x24
+	#define AX_MONITOR_MODE_RWLC	0x02
+	#define AX_MONITOR_MODE_RWMP	0x04
+	#define AX_MONITOR_MODE_PMEPOL	0x20
+	#define AX_MONITOR_MODE_PMETYPE	0x40
+
+#define AX_GPIO_CTRL				0x25
+	#define AX_GPIO_CTRL_GPIO3EN	0x80
+	#define AX_GPIO_CTRL_GPIO2EN	0x40
+	#define AX_GPIO_CTRL_GPIO1EN	0x20
+
+#define AX_PHYPWR_RSTCTL			0x26
+	#define AX_PHYPWR_RSTCTL_BZ	0x0010
+	#define AX_PHYPWR_RSTCTL_IPRL	0x0020
+	#define AX_PHYPWR_RSTCTL_AT	0x1000
+
+#define AX_RX_BULKIN_QCTRL			0x2e
+#define AX_CLK_SELECT				0x33
+	#define AX_CLK_SELECT_BCS	0x01
+	#define AX_CLK_SELECT_ACS	0x02
+	#define AX_CLK_SELECT_ULR	0x08
+
+#define AX_RXCOE_CTL				0x34
+	#define AX_RXCOE_IP		0x01
+	#define AX_RXCOE_TCP		0x02
+	#define AX_RXCOE_UDP		0x04
+	#define AX_RXCOE_TCPV6		0x20
+	#define AX_RXCOE_UDPV6		0x40
+
+#define AX_TXCOE_CTL				0x35
+	#define AX_TXCOE_IP		0x01
+	#define AX_TXCOE_TCP		0x02
+	#define AX_TXCOE_UDP		0x04
+	#define AX_TXCOE_TCPV6		0x20
+	#define AX_TXCOE_UDPV6		0x40
+
+#define AX_LEDCTRL				0x73
+
+#define GMII_PHY_PHYSR				0x11
+	#define GMII_PHY_PHYSR_SMASK	0xc000
+	#define GMII_PHY_PHYSR_GIGA	0x8000
+	#define GMII_PHY_PHYSR_100	0x4000
+	#define GMII_PHY_PHYSR_FULL	0x2000
+	#define GMII_PHY_PHYSR_LINK	0x400
+
+#define GMII_LED_ACT				0x1a
+	#define	GMII_LED_ACTIVE_MASK	0xff8f
+	#define	GMII_LED0_ACTIVE	(1 << 4)
+	#define	GMII_LED1_ACTIVE	(1 << 5)
+	#define	GMII_LED2_ACTIVE	(1 << 6)
+
+#define GMII_LED_LINK				0x1c
+	#define	GMII_LED_LINK_MASK	0xf888
+	#define	GMII_LED0_LINK_10	(1 << 0)
+	#define	GMII_LED0_LINK_100	(1 << 1)
+	#define	GMII_LED0_LINK_1000	(1 << 2)
+	#define	GMII_LED1_LINK_10	(1 << 4)
+	#define	GMII_LED1_LINK_100	(1 << 5)
+	#define	GMII_LED1_LINK_1000	(1 << 6)
+	#define	GMII_LED2_LINK_10	(1 << 8)
+	#define	GMII_LED2_LINK_100	(1 << 9)
+	#define	GMII_LED2_LINK_1000	(1 << 10)
+	#define	LED0_ACTIVE		(1 << 0)
+	#define	LED0_LINK_10		(1 << 1)
+	#define	LED0_LINK_100		(1 << 2)
+	#define	LED0_LINK_1000		(1 << 3)
+	#define	LED0_FD			(1 << 4)
+	#define	LED0_USB3_MASK		0x001f
+	#define	LED1_ACTIVE		(1 << 5)
+	#define	LED1_LINK_10		(1 << 6)
+	#define	LED1_LINK_100		(1 << 7)
+	#define	LED1_LINK_1000		(1 << 8)
+	#define	LED1_FD			(1 << 9)
+	#define	LED1_USB3_MASK		0x03e0
+	#define	LED2_ACTIVE		(1 << 10)
+	#define	LED2_LINK_1000		(1 << 13)
+	#define	LED2_LINK_100		(1 << 12)
+	#define	LED2_LINK_10		(1 << 11)
+	#define	LED2_FD			(1 << 14)
+	#define	LED_VALID		(1 << 15)
+	#define	LED2_USB3_MASK		0x7c00
+
+#define GMII_PHYPAGE				0x1e
+#define GMII_PHY_PAGE_SELECT			0x1f
+	#define GMII_PHY_PGSEL_EXT	0x0007
+	#define GMII_PHY_PGSEL_PAGE0	0x0000
+
+
+/* local defines */
+#define ASIX_BASE_NAME "axg"
+#define USB_CTRL_SET_TIMEOUT 12000
+#define USB_CTRL_GET_TIMEOUT 12000
+#define USB_BULK_SEND_TIMEOUT 12000
+#define USB_BULK_RECV_TIMEOUT 12000
+
+#define AX_RX_URB_SIZE 1024 * 0x12
+#define PHY_CONNECT_TIMEOUT 12000
+
+#define TIMEOUT_RESOLUTION 50	/* ms */
+
+
+#define FLAG_NONE			0
+#define FLAG_TYPE_AX88179	(1U << 0)
+#define FLAG_TYPE_AX88178a	(1U << 1)
+#define FLAG_TYPE_DLINK_DUB1312	(1U << 2)
+#define FLAG_TYPE_SITECOM	(1U << 3)
+#define FLAG_TYPE_SAMSUNG	(1U << 4)
+#define FLAG_TYPE_LENOVO	(1U << 5)
+
+static int asix_send(struct eth_device *eth, void *packet, int length);
+
+
+/* local vars */
+
+static const struct {
+	unsigned char ctrl, timer_l, timer_h, size, ifg;
+} AX88179_BULKIN_SIZE[] =	{
+	{7, 0x4f, 0,	0x12, 0xff},
+	{7, 0x20, 3,	0x16, 0xff},
+	{7, 0xae, 7,	0x18, 0xff},
+	{7, 0xcc, 0x4c, 0x18, 8},
+};
+
+static int curr_eth_dev; /* index for name of next device detected */
+
+/* driver private */
+struct asix_private {
+	int flags;
+};
+
+/*
+ * Asix infrastructure commands
+ */
+static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16 index,
+			     u16 size, void *data)
+{
+	int len;
+
+	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
+	      cmd, value, index, size);
+
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
+
+	memcpy(buf, data, size);
+
+	len = usb_control_msg(
+		dev->pusb_dev,
+		usb_sndctrlpipe(dev->pusb_dev, 0),
+		cmd,
+		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		value,
+		index,
+		buf,
+		size,
+		USB_CTRL_SET_TIMEOUT);
+
+	return len == size ? 0 : -1;
+}
+
+static int asix_read_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16 index,
+			    u16 size, void *data)
+{
+	int len;
+
+	debug("asix_read_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
+	      cmd, value, index, size);
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
+
+
+	len = usb_control_msg(
+		dev->pusb_dev,
+		usb_rcvctrlpipe(dev->pusb_dev, 0),
+		cmd,
+		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		value,
+		index,
+		buf,
+		size,
+		USB_CTRL_GET_TIMEOUT);
+
+	memcpy(data, buf, size);
+
+	return len == size ? 0 : -1;
+}
+
+
+static int asix_read_mac(struct eth_device *eth)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
+
+	if (dev->pusb_dev->descriptor.idProduct == 0x1790) {
+		asix_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, 6, 6, buf);
+		debug("asix_read_mac() returning %02x:%02x:%02x:%02x:%02x:%02x\n",
+		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
+		memcpy(eth->enetaddr, buf, ETH_ALEN);
+	}
+	return 0;
+}
+
+
+
+static int asix_basic_reset(struct ueth_data *dev)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
+	u16 *tmp16;
+	u8 *tmp;
+
+	tmp16 = (u16 *)buf;
+	tmp = (u8 *)buf;
+
+	/* Power up ethernet PHY */
+	*tmp16 = 0;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
+
+	*tmp16 = AX_PHYPWR_RSTCTL_IPRL;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
+	mdelay(200);
+
+	*tmp = AX_CLK_SELECT_ACS | AX_CLK_SELECT_BCS;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_CLK_SELECT, 1, 1, tmp);
+	mdelay(200);
+
+	/* RX bulk configuration */
+	memcpy(tmp, &AX88179_BULKIN_SIZE[0], 5);
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_RX_BULKIN_QCTRL, 5, 5, tmp);
+
+	/* Water Level configuration */
+	*tmp = 0x34;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 1, 1, tmp);
+
+	*tmp = 0x52;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_HIGH, 1, 1, tmp);
+
+	/* Enable checksum offload */
+	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
+	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_RXCOE_CTL, 1, 1, tmp);
+
+	*tmp = AX_TXCOE_IP | AX_TXCOE_TCP | AX_TXCOE_UDP |
+	       AX_TXCOE_TCPV6 | AX_TXCOE_UDPV6;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_TXCOE_CTL, 1, 1, tmp);
+
+	/* Configure RX control register => start operation */
+	*tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
+		 AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16);
+
+	*tmp = AX_MONITOR_MODE_PMETYPE | AX_MONITOR_MODE_PMEPOL |
+	       AX_MONITOR_MODE_RWMP;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD, 1, 1, tmp);
+
+	/* Configure default medium type => giga */
+	*tmp16 = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
+		 AX_MEDIUM_RXFLOW_CTRLEN | AX_MEDIUM_FULL_DUPLEX |
+		 AX_MEDIUM_GIGAMODE | AX_MEDIUM_JUMBO_EN;
+	asix_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, tmp16);
+
+	u16 adv = 0;
+	adv = ADVERTISE_ALL | ADVERTISE_CSMA | ADVERTISE_LPACK |
+	      ADVERTISE_NPAGE | ADVERTISE_PAUSE_ASYM | ADVERTISE_PAUSE_CAP;
+	asix_write_cmd(dev, AX_ACCESS_PHY, 0x03, MII_ADVERTISE, 2, &adv);
+
+	adv = ADVERTISE_1000FULL;
+	asix_write_cmd(dev, AX_ACCESS_PHY, 0x03, MII_CTRL1000, 2, &adv);
+
+	return 0;
+}
+
+/*
+ * Asix callbacks
+ */
+static int asix_init(struct eth_device *eth, bd_t *bd)
+{
+	struct ueth_data	*dev = (struct ueth_data *)eth->priv;
+	int timeout = 0;
+	int link_detected;
+
+	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
+	u16 *tmp16;
+
+	tmp16 = (u16 *)buf;
+
+	debug("** %s()\n", __func__);
+
+
+	/* Configure RX control register => start operation */
+	*tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
+		 AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
+	if (asix_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16) < 0)
+		goto out_err;
+
+	do {
+		asix_read_cmd(dev, AX_ACCESS_PHY, 0x03, MII_BMSR, 2, buf);
+		link_detected = *tmp16 & BMSR_LSTATUS;
+		if (!link_detected) {
+			if (timeout == 0)
+				printf("Waiting for Ethernet connection... ");
+			udelay(TIMEOUT_RESOLUTION * 1000);
+			timeout += TIMEOUT_RESOLUTION;
+		}
+	} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);
+	if (link_detected) {
+		if (timeout != 0)
+			printf("done.\n");
+			return 0;
+	} else {/*reset device and try again*/
+		printf("unable to connect.\n");
+		printf("Reset Ethernet Device\n");
+		asix_basic_reset(dev);
+		timeout = 0;
+		do {
+			asix_read_cmd(dev, AX_ACCESS_PHY, 0x03,
+				      MII_BMSR, 2, buf);
+			link_detected = *tmp16 & BMSR_LSTATUS;
+			if (!link_detected) {
+				if (timeout == 0)
+					printf("Waiting for Ethernet connection... ");
+				udelay(TIMEOUT_RESOLUTION * 1000);
+				timeout += TIMEOUT_RESOLUTION;
+			}
+		} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);
+		if (link_detected) {
+			if (timeout != 0)
+				printf("done.\n");
+				return 0;
+			} else {
+				printf("unable to connect.\n");
+				goto out_err;
+				}
+	}
+
+	return 0;
+out_err:
+	return -1;
+}
+
+static int asix_send(struct eth_device *eth, void *packet, int length)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	int err;
+	u32 packet_len, tx_hdr2;
+	int actual_len;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg,
+				 PKTSIZE + (2 * sizeof(packet_len)));
+
+	debug("** %s(), len %d\n", __func__, length);
+
+	packet_len = length;
+	cpu_to_le32s(&packet_len);
+
+	memcpy(msg, &packet_len, sizeof(packet_len));
+
+	tx_hdr2 = 0;
+	if (((length + 8) % 0x200 /*frame_size*/) == 0)
+		tx_hdr2 |= 0x80008000;	/* Enable padding */
+
+	cpu_to_le32s(&tx_hdr2);
+
+	memcpy(msg + sizeof(packet_len), &tx_hdr2, sizeof(tx_hdr2));
+
+	memcpy(msg + sizeof(packet_len) + sizeof(tx_hdr2),
+	       (void *)packet, length);
+
+	err = usb_bulk_msg(dev->pusb_dev,
+				usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
+				(void *)msg,
+				length + sizeof(packet_len) + sizeof(tx_hdr2),
+				&actual_len,
+				USB_BULK_SEND_TIMEOUT);
+	debug("Tx: len = %u, actual = %u, err = %d\n",
+	      length + sizeof(packet_len), actual_len, err);
+
+	return err;
+}
+
+static int asix_recv(struct eth_device *eth)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, recv_buf, AX_RX_URB_SIZE);
+	u16 frame_pos;
+	int err;
+	int actual_len;
+
+	int pkt_cnt;
+	u32 rx_hdr;
+	u16 hdr_off;
+	u32 *pkt_hdr;
+
+	actual_len = -1;
+
+	debug("** %s()\n", __func__);
+
+	err = usb_bulk_msg(dev->pusb_dev,
+				usb_rcvbulkpipe(dev->pusb_dev, dev->ep_in),
+				(void *)recv_buf,
+				AX_RX_URB_SIZE,
+				&actual_len,
+				USB_BULK_RECV_TIMEOUT);
+	debug("Rx: len = %u, actual = %u, err = %d\n", AX_RX_URB_SIZE,
+	      actual_len, err);
+
+	if (err != 0) {
+		debug("Rx: failed to receive\n");
+		return -1;
+	}
+	if (actual_len > AX_RX_URB_SIZE) {
+		debug("Rx: received too many bytes %d\n", actual_len);
+		return -1;
+	}
+
+
+	rx_hdr = *(u32 *)(recv_buf + actual_len - 4);
+	le32_to_cpus(&pkt_hdr);
+
+	pkt_cnt = (u16)rx_hdr;
+	hdr_off = (u16)(rx_hdr >> 16);
+	pkt_hdr = (u32 *)(recv_buf + hdr_off);
+
+
+	frame_pos = 0;
+
+	while (pkt_cnt--) {
+		u16 pkt_len;
+
+		le32_to_cpus(pkt_hdr);
+		pkt_len = (*pkt_hdr >> 16) & 0x1fff;
+
+		frame_pos += 2;
+
+		NetReceive(recv_buf + frame_pos, pkt_len);
+
+		pkt_hdr++;
+		frame_pos += ((pkt_len + 7) & 0xFFF8)-2;
+
+		if (pkt_cnt == 0)
+			return 1;
+	}
+	return err;
+}
+
+static void asix_halt(struct eth_device *eth)
+{
+	debug("** %s()\n", __func__);
+}
+
+/*
+ * Asix probing functions
+ */
+void ax88179_eth_before_probe(void)
+{
+	curr_eth_dev = 0;
+}
+
+struct asix_dongle {
+	unsigned short vendor;
+	unsigned short product;
+	int flags;
+};
+
+static const struct asix_dongle asix_dongles[] = {
+	{ 0x0b95, 0x1790, FLAG_TYPE_AX88179 },
+	{ 0x0b95, 0x178a, FLAG_TYPE_AX88178a },
+	{ 0x2001, 0x4a00, FLAG_TYPE_DLINK_DUB1312 },
+	{ 0x0df6, 0x0072, FLAG_TYPE_SITECOM },
+	{ 0x04e8, 0xa100, FLAG_TYPE_SAMSUNG },
+	{ 0x17ef, 0x304b, FLAG_TYPE_LENOVO },
+	{ 0x0000, 0x0000, FLAG_NONE }	/* END - Do not remove */
+};
+
+/* Probe to see if a new device is actually an asix device */
+int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss)
+{
+	struct usb_interface *iface;
+	struct usb_interface_descriptor *iface_desc;
+	int ep_in_found = 0, ep_out_found = 0;
+	int i;
+
+	/* let's examine the device now */
+	iface = &dev->config.if_desc[ifnum];
+	iface_desc = &dev->config.if_desc[ifnum].desc;
+
+	for (i = 0; asix_dongles[i].vendor != 0; i++) {
+		if (dev->descriptor.idVendor == asix_dongles[i].vendor &&
+		    dev->descriptor.idProduct == asix_dongles[i].product)
+			/* Found a supported dongle */
+			break;
+	}
+
+	if (asix_dongles[i].vendor == 0)
+		return 0;
+
+	memset(ss, 0, sizeof(struct ueth_data));
+
+	/* At this point, we know we've got a live one */
+	debug("\n\nUSB Ethernet device detected: %#04x:%#04x\n",
+	      dev->descriptor.idVendor, dev->descriptor.idProduct);
+
+	/* Initialize the ueth_data structure with some useful info */
+	ss->ifnum = ifnum;
+	ss->pusb_dev = dev;
+	ss->subclass = iface_desc->bInterfaceSubClass;
+	ss->protocol = iface_desc->bInterfaceProtocol;
+
+	/* alloc driver private */
+	ss->dev_priv = calloc(1, sizeof(struct asix_private));
+	if (!ss->dev_priv)
+		return 0;
+
+	((struct asix_private *)ss->dev_priv)->flags = asix_dongles[i].flags;
+
+	/*
+	 * We are expecting a minimum of 3 endpoints - in, out (bulk), and
+	 * int. We will ignore any others.
+	 */
+	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
+		/* is it an BULK endpoint? */
+		if ((iface->ep_desc[i].bmAttributes &
+		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
+			u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
+			if (ep_addr & USB_DIR_IN) {
+				if (!ep_in_found) {
+					ss->ep_in = ep_addr &
+						USB_ENDPOINT_NUMBER_MASK;
+					ep_in_found = 1;
+				}
+			} else {
+				if (!ep_out_found) {
+					ss->ep_out = ep_addr &
+						USB_ENDPOINT_NUMBER_MASK;
+					ep_out_found = 1;
+				}
+			}
+		}
+
+		/* is it an interrupt endpoint? */
+		if ((iface->ep_desc[i].bmAttributes &
+		    USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
+			ss->ep_int = iface->ep_desc[i].bEndpointAddress &
+				USB_ENDPOINT_NUMBER_MASK;
+			ss->irqinterval = iface->ep_desc[i].bInterval;
+		}
+	}
+	debug("Endpoints In %d Out %d Int %d\n",
+	      ss->ep_in, ss->ep_out, ss->ep_int);
+
+	/* Do some basic sanity checks, and bail if we find a problem */
+	if (usb_set_interface(dev, iface_desc->bInterfaceNumber, 0) ||
+	    !ss->ep_in || !ss->ep_out || !ss->ep_int) {
+		debug("Problems with device\n");
+		return 0;
+	}
+	dev->privptr = (void *)ss;
+	return 1;
+}
+
+int ax88179_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
+				struct eth_device *eth)
+{
+	if (!eth) {
+		debug("%s: missing parameter.\n", __func__);
+		return 0;
+	}
+	sprintf(eth->name, "%s%d", ASIX_BASE_NAME, curr_eth_dev++);
+	eth->init = asix_init;
+	eth->send = asix_send;
+	eth->recv = asix_recv;
+	eth->halt = asix_halt;
+	eth->priv = ss;
+
+	if (asix_basic_reset(ss))
+		return 0;
+
+	/* Get the MAC address */
+	if (asix_read_mac(eth))
+		return 0;
+	debug("MAC %pM\n", eth->enetaddr);
+
+	return 1;
+}
diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
index 1dda54c..7cb96e3 100644
--- a/drivers/usb/eth/usb_ether.c
+++ b/drivers/usb/eth/usb_ether.c
@@ -30,6 +30,13 @@ static const struct usb_eth_prob_dev prob_dev[] = {
 		.get_info = asix_eth_get_info,
 	},
 #endif
+#ifdef CONFIG_USB_ETHER_ASIX88179
+	{
+		.before_probe = ax88179_eth_before_probe,
+		.probe = ax88179_eth_probe,
+		.get_info = ax88179_eth_get_info,
+	},
+#endif
 #ifdef CONFIG_USB_ETHER_MCS7830
 	{
 		.before_probe = mcs7830_eth_before_probe,
diff --git a/include/usb_ether.h b/include/usb_ether.h
index 35700a2..b38d037 100644
--- a/include/usb_ether.h
+++ b/include/usb_ether.h
@@ -49,6 +49,12 @@ int asix_eth_probe(struct usb_device *dev, unsigned int ifnum,
 int asix_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
 		      struct eth_device *eth);
 
+void ax88179_eth_before_probe(void);
+int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss);
+int ax88179_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
+		      struct eth_device *eth);
+
 void mcs7830_eth_before_probe(void);
 int mcs7830_eth_probe(struct usb_device *dev, unsigned int ifnum,
 		      struct ueth_data *ss);
-- 
1.9.1

[U-Boot] [PATCH V3 3/3] usb: eth: enable AX88179 DRIVER for ARNDALE 5250

[Rene Griessl]

enable this driver for arndale5250 board

Signed-off-by: Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
---
 include/configs/arndale.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/arndale.h b/include/configs/arndale.h
index 75f9933..cf5c39e 100644
--- a/include/configs/arndale.h
+++ b/include/configs/arndale.h
@@ -118,6 +118,7 @@
 #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS	3
 #define CONFIG_USB_HOST_ETHER
 #define CONFIG_USB_ETHER_ASIX
+#define CONFIG_USB_ETHER_ASIX88179
 
 /* MMC SPL */
 #define CONFIG_EXYNOS_SPL
-- 
1.9.1

[U-Boot] [PATCH V3 1/3] usb: eth: fix Makefile

[Marek Vasut]

On Friday, September 26, 2014 at 10:08:47 AM, Rene Griessl wrote:
> fix issue as discussed with Marek Vasut

Now I'm twice as famous ;-)

But in fact, you should actually explain the change you did in the commit 
message (aka. something along the lines of the ifdef being useless and that
using simpler and consistent notation is fine).

> Signed-off-by: Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
> ---
>  drivers/usb/eth/Makefile | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
> index 94551c4..e6ae9f1 100644
> --- a/drivers/usb/eth/Makefile
> +++ b/drivers/usb/eth/Makefile
> @@ -5,8 +5,6 @@
> 
>  # new USB host ethernet layer dependencies
>  obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
> -ifdef CONFIG_USB_ETHER_ASIX
> -obj-y += asix.o
> -endif
> +obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
>  obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
>  obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o

Best regards,
Marek Vasut

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[Marek Vasut]

On Friday, September 26, 2014 at 10:08:48 AM, Rene Griessl wrote:
> changes in v3:
> 	-added all compatible devices from linux driver
> 	-fixed issues from review
> 
> changes in v2:
>         -added usb_ether.h to change list
>         -added 2nd patch to enable driver for arndale board

The changelog goes to the [*] marker below. And you're missing a meaningful 
commit message too. Also, if the driver is pulled from Linux, please specify
from which commit in there, so future developers might re-sync the driver if 
needed be and they'd know from which point the driver was derived.

> Signed-off-by: Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
> ---
>  drivers/usb/eth/Makefile    |   1 +
>  drivers/usb/eth/asix88179.c | 659
> ++++++++++++++++++++++++++++++++++++++++++++ drivers/usb/eth/usb_ether.c |
>   7 +
>  include/usb_ether.h         |   6 +
>  4 files changed, 673 insertions(+)
>  create mode 100644 drivers/usb/eth/asix88179.c
> 
> diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
> index e6ae9f1..c92d2b0 100644
> --- a/drivers/usb/eth/Makefile
> +++ b/drivers/usb/eth/Makefile
> @@ -6,5 +6,6 @@
>  # new USB host ethernet layer dependencies
>  obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
>  obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
> +obj-$(CONFIG_USB_ETHER_ASIX88179) += asix88179.o
>  obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
>  obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
> diff --git a/drivers/usb/eth/asix88179.c b/drivers/usb/eth/asix88179.c
> new file mode 100644
> index 0000000..2079056
> --- /dev/null
> +++ b/drivers/usb/eth/asix88179.c
> @@ -0,0 +1,659 @@
> +/*
> + * Copyright (c) 2014 Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
> + * based on the U-Boot Asix driver as well as information
> + * from the Linux AX88179_178a driver
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +

One newline too many here.

> +#include <common.h>
> +#include <usb.h>
> +#include <net.h>
> +#include <linux/mii.h>
> +#include "usb_ether.h"
> +#include <malloc.h>
> +
> +

DTTO

> +/* ASIX AX88179 based USB 3.0 Ethernet Devices */
> +#define AX88179_PHY_ID				0x03
> +#define AX_EEPROM_LEN				0x100
> +#define AX88179_EEPROM_MAGIC			0x17900b95
> +#define AX_MCAST_FLTSIZE			8
> +#define AX_MAX_MCAST				64
> +#define AX_INT_PPLS_LINK			(1 << 16)
> +#define AX_RXHDR_L4_TYPE_MASK			0x1c
> +#define AX_RXHDR_L4_TYPE_UDP			4
> +#define AX_RXHDR_L4_TYPE_TCP			16
> +#define AX_RXHDR_L3CSUM_ERR			2
> +#define AX_RXHDR_L4CSUM_ERR			1
> +#define AX_RXHDR_CRC_ERR			(1 << 29)
> +#define AX_RXHDR_DROP_ERR			(1 << 31)
> +#define AX_ACCESS_MAC				0x01
> +#define AX_ACCESS_PHY				0x02
> +#define AX_ACCESS_EEPROM			0x04
> +#define AX_ACCESS_EFUS				0x05
> +#define AX_PAUSE_WATERLVL_HIGH			0x54
> +#define AX_PAUSE_WATERLVL_LOW			0x55
> +
> +#define PHYSICAL_LINK_STATUS			0x02
> +	#define	AX_USB_SS		0x04
> +	#define	AX_USB_HS		0x02
> +
> +#define GENERAL_STATUS				0x03
> +/* Check AX88179 version. UA1:Bit2 = 0,  UA2:Bit2 = 1 */
> +	#define	AX_SECLD		0x04
> +
> +#define AX_SROM_ADDR				0x07
> +#define AX_SROM_CMD				0x0a
> +	#define EEP_RD			0x04
> +	#define EEP_BUSY		0x10

If those are bits, then just use (1 << n) notation.
[...]

> +static int curr_eth_dev; /* index for name of next device detected */
> +
> +/* driver private */
> +struct asix_private {
> +	int flags;
> +};

This thing is a little iffy ... do you really need a full-blown struct here or 
can you just use array ?

> +/*
> + * Asix infrastructure commands
> + */
> +static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
> index, +			     u16 size, void *data)
> +{
> +	int len;
> +
> +	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
> +	      cmd, value, index, size);
> +
> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);

I think if you really enable the debug to generate a printf() , the compiler 
will spew that you wrote code before variable declaration.

> +	memcpy(buf, data, size);
> +
> +	len = usb_control_msg(
> +		dev->pusb_dev,
> +		usb_sndctrlpipe(dev->pusb_dev, 0),
> +		cmd,
> +		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +		value,
> +		index,
> +		buf,
> +		size,
> +		USB_CTRL_SET_TIMEOUT);
> +
> +	return len == size ? 0 : -1;

Use values from errno.h here ?

> +}
> +
> +static int asix_read_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
> index, +			    u16 size, void *data)
> +{
> +	int len;
> +
> +	debug("asix_read_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
> +	      cmd, value, index, size);
> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
> +
> +
> +	len = usb_control_msg(
> +		dev->pusb_dev,
> +		usb_rcvctrlpipe(dev->pusb_dev, 0),
> +		cmd,
> +		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +		value,
> +		index,
> +		buf,
> +		size,
> +		USB_CTRL_GET_TIMEOUT);
> +
> +	memcpy(data, buf, size);
> +
> +	return len == size ? 0 : -1;
> +}
> +
> +
> +static int asix_read_mac(struct eth_device *eth)
> +{
> +	struct ueth_data *dev = (struct ueth_data *)eth->priv;
> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
> +
> +	if (dev->pusb_dev->descriptor.idProduct == 0x1790) {
> +		asix_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, 6, 6, buf);
> +		debug("asix_read_mac() returning %02x:%02x:%02x:%02x:%02x:
%02x\n",
> +		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
> +		memcpy(eth->enetaddr, buf, ETH_ALEN);
> +	}
> +	return 0;
> +}
> +
> +
> +
> +static int asix_basic_reset(struct ueth_data *dev)
> +{
> +	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
> +	u16 *tmp16;
> +	u8 *tmp;
> +
> +	tmp16 = (u16 *)buf;
> +	tmp = (u8 *)buf;
> +
> +	/* Power up ethernet PHY */
> +	*tmp16 = 0;
> +	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);

The asix_write_cmd() has some bounce-buffer logic in it already, does the tmp16 
need to be aligned here too? Also, you might want to use include/bouncebuf.h and 
friends instead of implementing your own bounce buffering.

[...]

> +/*
> + * Asix callbacks
> + */
> +static int asix_init(struct eth_device *eth, bd_t *bd)
> +{
> +	struct ueth_data	*dev = (struct ueth_data *)eth->priv;
> +	int timeout = 0;
> +	int link_detected;
> +
> +	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
> +	u16 *tmp16;
> +
> +	tmp16 = (u16 *)buf;
> +
> +	debug("** %s()\n", __func__);
> +
> +
> +	/* Configure RX control register => start operation */
> +	*tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
> +		 AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
> +	if (asix_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16) < 0)
> +		goto out_err;
> +
> +	do {
> +		asix_read_cmd(dev, AX_ACCESS_PHY, 0x03, MII_BMSR, 2, buf);
> +		link_detected = *tmp16 & BMSR_LSTATUS;
> +		if (!link_detected) {
> +			if (timeout == 0)
> +				printf("Waiting for Ethernet connection... ");
> +			udelay(TIMEOUT_RESOLUTION * 1000);

mdelay()

> +			timeout += TIMEOUT_RESOLUTION;
> +		}
> +	} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);

Newline

> +	if (link_detected) {
> +		if (timeout != 0)
> +			printf("done.\n");
> +			return 0;

Where does this return 0; belong to ?

> +	} else {/*reset device and try again*/
> +		printf("unable to connect.\n");
> +		printf("Reset Ethernet Device\n");
> +		asix_basic_reset(dev);
> +		timeout = 0;
> +		do {
> +			asix_read_cmd(dev, AX_ACCESS_PHY, 0x03,
> +				      MII_BMSR, 2, buf);
> +			link_detected = *tmp16 & BMSR_LSTATUS;
> +			if (!link_detected) {
> +				if (timeout == 0)
> +					printf("Waiting for Ethernet 
connection... ");
> +				udelay(TIMEOUT_RESOLUTION * 1000);

mdelay()

> +				timeout += TIMEOUT_RESOLUTION;
> +			}
> +		} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);
> +		if (link_detected) {
> +			if (timeout != 0)
> +				printf("done.\n");
> +				return 0;
> +			} else {
> +				printf("unable to connect.\n");
> +				goto out_err;
> +				}

The indent is crazy in here ...

> +	}
> +
> +	return 0;
> +out_err:
> +	return -1;
> +}
> +
> +static int asix_send(struct eth_device *eth, void *packet, int length)
> +{
> +	struct ueth_data *dev = (struct ueth_data *)eth->priv;
> +	int err;
> +	u32 packet_len, tx_hdr2;
> +	int actual_len;
> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg,
> +				 PKTSIZE + (2 * sizeof(packet_len)));
> +
> +	debug("** %s(), len %d\n", __func__, length);
> +
> +	packet_len = length;
> +	cpu_to_le32s(&packet_len);
> +
> +	memcpy(msg, &packet_len, sizeof(packet_len));
> +
> +	tx_hdr2 = 0;
> +	if (((length + 8) % 0x200 /*frame_size*/) == 0)

Define the frame size as a named constant, then use it here.

> +		tx_hdr2 |= 0x80008000;	/* Enable padding */
> +
> +	cpu_to_le32s(&tx_hdr2);
> +
> +	memcpy(msg + sizeof(packet_len), &tx_hdr2, sizeof(tx_hdr2));
> +
> +	memcpy(msg + sizeof(packet_len) + sizeof(tx_hdr2),
> +	       (void *)packet, length);
> +
> +	err = usb_bulk_msg(dev->pusb_dev,
> +				usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
> +				(void *)msg,
> +				length + sizeof(packet_len) + sizeof(tx_hdr2),
> +				&actual_len,
> +				USB_BULK_SEND_TIMEOUT);
> +	debug("Tx: len = %u, actual = %u, err = %d\n",
> +	      length + sizeof(packet_len), actual_len, err);
> +
> +	return err;
> +}

[...]

> +/* Probe to see if a new device is actually an asix device */
> +int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
> +		      struct ueth_data *ss)
> +{
> +	struct usb_interface *iface;
> +	struct usb_interface_descriptor *iface_desc;
> +	int ep_in_found = 0, ep_out_found = 0;
> +	int i;
> +
> +	/* let's examine the device now */
> +	iface = &dev->config.if_desc[ifnum];
> +	iface_desc = &dev->config.if_desc[ifnum].desc;
> +
> +	for (i = 0; asix_dongles[i].vendor != 0; i++) {
> +		if (dev->descriptor.idVendor == asix_dongles[i].vendor &&
> +		    dev->descriptor.idProduct == asix_dongles[i].product)
> +			/* Found a supported dongle */
> +			break;
> +	}
> +
> +	if (asix_dongles[i].vendor == 0)
> +		return 0;
> +
> +	memset(ss, 0, sizeof(struct ueth_data));
> +
> +	/* At this point, we know we've got a live one */
> +	debug("\n\nUSB Ethernet device detected: %#04x:%#04x\n",
> +	      dev->descriptor.idVendor, dev->descriptor.idProduct);
> +
> +	/* Initialize the ueth_data structure with some useful info */
> +	ss->ifnum = ifnum;
> +	ss->pusb_dev = dev;
> +	ss->subclass = iface_desc->bInterfaceSubClass;
> +	ss->protocol = iface_desc->bInterfaceProtocol;
> +
> +	/* alloc driver private */
> +	ss->dev_priv = calloc(1, sizeof(struct asix_private));
> +	if (!ss->dev_priv)
> +		return 0;
> +
> +	((struct asix_private *)ss->dev_priv)->flags = asix_dongles[i].flags;
> +
> +	/*
> +	 * We are expecting a minimum of 3 endpoints - in, out (bulk), and
> +	 * int. We will ignore any others.
> +	 */
> +	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
> +		/* is it an BULK endpoint? */
> +		if ((iface->ep_desc[i].bmAttributes &
> +		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {

if (! ...)
 continue;

if ((ep_addr & USB_DIR_IN) && !ep_in_found) {
 something
}

if (!(ep_addr & USB_DIR_IN) && !ep_out_found) {
 something
}
> +			u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
> +			if (ep_addr & USB_DIR_IN) {
> +				if (!ep_in_found) {
> +					ss->ep_in = ep_addr &
> +						USB_ENDPOINT_NUMBER_MASK;
> +					ep_in_found = 1;
> +				}
> +			} else {
> +				if (!ep_out_found) {
> +					ss->ep_out = ep_addr &
> +						USB_ENDPOINT_NUMBER_MASK;
> +					ep_out_found = 1;
> +				}
> +			}

See above how to trim down the indent here.
[...]

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[René Griessl]

>> changes in v3:
>> 	-added all compatible devices from linux driver
>> 	-fixed issues from review
>>
>> changes in v2:
>>          -added usb_ether.h to change list
>>          -added 2nd patch to enable driver for arndale board
> The changelog goes to the [*] marker below. And you're missing a meaningful
> commit message too. Also, if the driver is pulled from Linux, please specify
> from which commit in there, so future developers might re-sync the driver if
> needed be and they'd know from which point the driver was derived.
Were exactly can I find the marker?
>> Signed-off-by: Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
>> ---
>>   drivers/usb/eth/Makefile    |   1 +
>>   drivers/usb/eth/asix88179.c | 659
>> ++++++++++++++++++++++++++++++++++++++++++++ drivers/usb/eth/usb_ether.c |
>>    7 +
>>   include/usb_ether.h         |   6 +
>>   4 files changed, 673 insertions(+)
>>   create mode 100644 drivers/usb/eth/asix88179.c
>>
>> diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
>> index e6ae9f1..c92d2b0 100644
>> --- a/drivers/usb/eth/Makefile
>> +++ b/drivers/usb/eth/Makefile
>> @@ -6,5 +6,6 @@
>>   # new USB host ethernet layer dependencies
>>   obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
>>   obj-$(CONFIG_USB_ETHER_ASIX) += asix.o
>> +obj-$(CONFIG_USB_ETHER_ASIX88179) += asix88179.o
>>   obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o
>>   obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
>> diff --git a/drivers/usb/eth/asix88179.c b/drivers/usb/eth/asix88179.c
>> new file mode 100644
>> index 0000000..2079056
>> --- /dev/null
>> +++ b/drivers/usb/eth/asix88179.c
>> @@ -0,0 +1,659 @@
>> +/*
>> + * Copyright (c) 2014 Rene Griessl <rgriessl@cit-ec.uni-bielefeld.de>
>> + * based on the U-Boot Asix driver as well as information
>> + * from the Linux AX88179_178a driver
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +
> One newline too many here.
>
>> +#include <common.h>
>> +#include <usb.h>
>> +#include <net.h>
>> +#include <linux/mii.h>
>> +#include "usb_ether.h"
>> +#include <malloc.h>
>> +
>> +
> DTTO
>
>> +/* ASIX AX88179 based USB 3.0 Ethernet Devices */
>> +#define AX88179_PHY_ID				0x03
>> +#define AX_EEPROM_LEN				0x100
>> +#define AX88179_EEPROM_MAGIC			0x17900b95
>> +#define AX_MCAST_FLTSIZE			8
>> +#define AX_MAX_MCAST				64
>> +#define AX_INT_PPLS_LINK			(1 << 16)
>> +#define AX_RXHDR_L4_TYPE_MASK			0x1c
>> +#define AX_RXHDR_L4_TYPE_UDP			4
>> +#define AX_RXHDR_L4_TYPE_TCP			16
>> +#define AX_RXHDR_L3CSUM_ERR			2
>> +#define AX_RXHDR_L4CSUM_ERR			1
>> +#define AX_RXHDR_CRC_ERR			(1 << 29)
>> +#define AX_RXHDR_DROP_ERR			(1 << 31)
>> +#define AX_ACCESS_MAC				0x01
>> +#define AX_ACCESS_PHY				0x02
>> +#define AX_ACCESS_EEPROM			0x04
>> +#define AX_ACCESS_EFUS				0x05
>> +#define AX_PAUSE_WATERLVL_HIGH			0x54
>> +#define AX_PAUSE_WATERLVL_LOW			0x55
>> +
>> +#define PHYSICAL_LINK_STATUS			0x02
>> +	#define	AX_USB_SS		0x04
>> +	#define	AX_USB_HS		0x02
>> +
>> +#define GENERAL_STATUS				0x03
>> +/* Check AX88179 version. UA1:Bit2 = 0,  UA2:Bit2 = 1 */
>> +	#define	AX_SECLD		0x04
>> +
>> +#define AX_SROM_ADDR				0x07
>> +#define AX_SROM_CMD				0x0a
>> +	#define EEP_RD			0x04
>> +	#define EEP_BUSY		0x10
> If those are bits, then just use (1 << n) notation.
> [...]
>
>> +static int curr_eth_dev; /* index for name of next device detected */
>> +
>> +/* driver private */
>> +struct asix_private {
>> +	int flags;
>> +};
> This thing is a little iffy ... do you really need a full-blown struct here or
> can you just use array ?
This struct is used to cast the flags to the U-Boot ueth driver. (see 
line 589 of c-file)
>> +/*
>> + * Asix infrastructure commands
>> + */
>> +static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
>> index, +			     u16 size, void *data)
>> +{
>> +	int len;
>> +
>> +	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
>> +	      cmd, value, index, size);
>> +
>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
> I think if you really enable the debug to generate a printf() , the compiler
> will spew that you wrote code before variable declaration.
Really? I took all of the variables from the function call. So with one 
has not declaration?
>> +	memcpy(buf, data, size);
>> +
>> +	len = usb_control_msg(
>> +		dev->pusb_dev,
>> +		usb_sndctrlpipe(dev->pusb_dev, 0),
>> +		cmd,
>> +		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>> +		value,
>> +		index,
>> +		buf,
>> +		size,
>> +		USB_CTRL_SET_TIMEOUT);
>> +
>> +	return len == size ? 0 : -1;
> Use values from errno.h here ?
>
>> +}
>> +
>> +static int asix_read_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
>> index, +			    u16 size, void *data)
>> +{
>> +	int len;
>> +
>> +	debug("asix_read_cmd() cmd=0x%02x value=0x%04x index=0x%04x size=%d\n",
>> +	      cmd, value, index, size);
>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
>> +
>> +
>> +	len = usb_control_msg(
>> +		dev->pusb_dev,
>> +		usb_rcvctrlpipe(dev->pusb_dev, 0),
>> +		cmd,
>> +		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>> +		value,
>> +		index,
>> +		buf,
>> +		size,
>> +		USB_CTRL_GET_TIMEOUT);
>> +
>> +	memcpy(data, buf, size);
>> +
>> +	return len == size ? 0 : -1;
>> +}
>> +
>> +
>> +static int asix_read_mac(struct eth_device *eth)
>> +{
>> +	struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
>> +
>> +	if (dev->pusb_dev->descriptor.idProduct == 0x1790) {
>> +		asix_read_cmd(dev, AX_ACCESS_MAC, AX_NODE_ID, 6, 6, buf);
>> +		debug("asix_read_mac() returning %02x:%02x:%02x:%02x:%02x:
> %02x\n",
>> +		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
>> +		memcpy(eth->enetaddr, buf, ETH_ALEN);
>> +	}
>> +	return 0;
>> +}
>> +
>> +
>> +
>> +static int asix_basic_reset(struct ueth_data *dev)
>> +{
>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
>> +	u16 *tmp16;
>> +	u8 *tmp;
>> +
>> +	tmp16 = (u16 *)buf;
>> +	tmp = (u8 *)buf;
>> +
>> +	/* Power up ethernet PHY */
>> +	*tmp16 = 0;
>> +	asix_write_cmd(dev, AX_ACCESS_MAC, AX_PHYPWR_RSTCTL, 2, 2, tmp16);
> The asix_write_cmd() has some bounce-buffer logic in it already, does the tmp16
> need to be aligned here too? Also, you might want to use include/bouncebuf.h and
> friends instead of implementing your own bounce buffering.
I will do some rework here...
> [...]
>
>> +/*
>> + * Asix callbacks
>> + */
>> +static int asix_init(struct eth_device *eth, bd_t *bd)
>> +{
>> +	struct ueth_data	*dev = (struct ueth_data *)eth->priv;
>> +	int timeout = 0;
>> +	int link_detected;
>> +
>> +	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
>> +	u16 *tmp16;
>> +
>> +	tmp16 = (u16 *)buf;
>> +
>> +	debug("** %s()\n", __func__);
>> +
>> +
>> +	/* Configure RX control register => start operation */
>> +	*tmp16 = AX_RX_CTL_DROPCRCERR | AX_RX_CTL_IPE | AX_RX_CTL_START |
>> +		 AX_RX_CTL_AP | AX_RX_CTL_AMALL | AX_RX_CTL_AB;
>> +	if (asix_write_cmd(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 2, tmp16) < 0)
>> +		goto out_err;
>> +
>> +	do {
>> +		asix_read_cmd(dev, AX_ACCESS_PHY, 0x03, MII_BMSR, 2, buf);
>> +		link_detected = *tmp16 & BMSR_LSTATUS;
>> +		if (!link_detected) {
>> +			if (timeout == 0)
>> +				printf("Waiting for Ethernet connection... ");
>> +			udelay(TIMEOUT_RESOLUTION * 1000);
> mdelay()
>
>> +			timeout += TIMEOUT_RESOLUTION;
>> +		}
>> +	} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);
> Newline
>
>> +	if (link_detected) {
>> +		if (timeout != 0)
>> +			printf("done.\n");
>> +			return 0;
> Where does this return 0; belong to ?
it quits the init function, because a link is detected. Is it more 
likely to put a goto here?
>> +	} else {/*reset device and try again*/
>> +		printf("unable to connect.\n");
>> +		printf("Reset Ethernet Device\n");
>> +		asix_basic_reset(dev);
>> +		timeout = 0;
>> +		do {
>> +			asix_read_cmd(dev, AX_ACCESS_PHY, 0x03,
>> +				      MII_BMSR, 2, buf);
>> +			link_detected = *tmp16 & BMSR_LSTATUS;
>> +			if (!link_detected) {
>> +				if (timeout == 0)
>> +					printf("Waiting for Ethernet
> connection... ");
>> +				udelay(TIMEOUT_RESOLUTION * 1000);
> mdelay()
>
>> +				timeout += TIMEOUT_RESOLUTION;
>> +			}
>> +		} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);
>> +		if (link_detected) {
>> +			if (timeout != 0)
>> +				printf("done.\n");
>> +				return 0;
>> +			} else {
>> +				printf("unable to connect.\n");
>> +				goto out_err;
>> +				}
> The indent is crazy in here ...
I will put the link detect routine in a separate function.
>> +	}
>> +
>> +	return 0;
>> +out_err:
>> +	return -1;
>> +}
>> +
>> +static int asix_send(struct eth_device *eth, void *packet, int length)
>> +{
>> +	struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> +	int err;
>> +	u32 packet_len, tx_hdr2;
>> +	int actual_len;
>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, msg,
>> +				 PKTSIZE + (2 * sizeof(packet_len)));
>> +
>> +	debug("** %s(), len %d\n", __func__, length);
>> +
>> +	packet_len = length;
>> +	cpu_to_le32s(&packet_len);
>> +
>> +	memcpy(msg, &packet_len, sizeof(packet_len));
>> +
>> +	tx_hdr2 = 0;
>> +	if (((length + 8) % 0x200 /*frame_size*/) == 0)
> Define the frame size as a named constant, then use it here.
>
>> +		tx_hdr2 |= 0x80008000;	/* Enable padding */
>> +
>> +	cpu_to_le32s(&tx_hdr2);
>> +
>> +	memcpy(msg + sizeof(packet_len), &tx_hdr2, sizeof(tx_hdr2));
>> +
>> +	memcpy(msg + sizeof(packet_len) + sizeof(tx_hdr2),
>> +	       (void *)packet, length);
>> +
>> +	err = usb_bulk_msg(dev->pusb_dev,
>> +				usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
>> +				(void *)msg,
>> +				length + sizeof(packet_len) + sizeof(tx_hdr2),
>> +				&actual_len,
>> +				USB_BULK_SEND_TIMEOUT);
>> +	debug("Tx: len = %u, actual = %u, err = %d\n",
>> +	      length + sizeof(packet_len), actual_len, err);
>> +
>> +	return err;
>> +}
> [...]
>
>> +/* Probe to see if a new device is actually an asix device */
>> +int ax88179_eth_probe(struct usb_device *dev, unsigned int ifnum,
>> +		      struct ueth_data *ss)
>> +{
>> +	struct usb_interface *iface;
>> +	struct usb_interface_descriptor *iface_desc;
>> +	int ep_in_found = 0, ep_out_found = 0;
>> +	int i;
>> +
>> +	/* let's examine the device now */
>> +	iface = &dev->config.if_desc[ifnum];
>> +	iface_desc = &dev->config.if_desc[ifnum].desc;
>> +
>> +	for (i = 0; asix_dongles[i].vendor != 0; i++) {
>> +		if (dev->descriptor.idVendor == asix_dongles[i].vendor &&
>> +		    dev->descriptor.idProduct == asix_dongles[i].product)
>> +			/* Found a supported dongle */
>> +			break;
>> +	}
>> +
>> +	if (asix_dongles[i].vendor == 0)
>> +		return 0;
>> +
>> +	memset(ss, 0, sizeof(struct ueth_data));
>> +
>> +	/* At this point, we know we've got a live one */
>> +	debug("\n\nUSB Ethernet device detected: %#04x:%#04x\n",
>> +	      dev->descriptor.idVendor, dev->descriptor.idProduct);
>> +
>> +	/* Initialize the ueth_data structure with some useful info */
>> +	ss->ifnum = ifnum;
>> +	ss->pusb_dev = dev;
>> +	ss->subclass = iface_desc->bInterfaceSubClass;
>> +	ss->protocol = iface_desc->bInterfaceProtocol;
>> +
>> +	/* alloc driver private */
>> +	ss->dev_priv = calloc(1, sizeof(struct asix_private));
>> +	if (!ss->dev_priv)
>> +		return 0;
>> +
>> +	((struct asix_private *)ss->dev_priv)->flags = asix_dongles[i].flags;
>> +
>> +	/*
>> +	 * We are expecting a minimum of 3 endpoints - in, out (bulk), and
>> +	 * int. We will ignore any others.
>> +	 */
>> +	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
>> +		/* is it an BULK endpoint? */
>> +		if ((iface->ep_desc[i].bmAttributes &
>> +		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
> if (! ...)
>   continue;
>
> if ((ep_addr & USB_DIR_IN) && !ep_in_found) {
>   something
> }
>
> if (!(ep_addr & USB_DIR_IN) && !ep_out_found) {
>   something
> }
>> +			u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
>> +			if (ep_addr & USB_DIR_IN) {
>> +				if (!ep_in_found) {
>> +					ss->ep_in = ep_addr &
>> +						USB_ENDPOINT_NUMBER_MASK;
>> +					ep_in_found = 1;
>> +				}
>> +			} else {
>> +				if (!ep_out_found) {
>> +					ss->ep_out = ep_addr &
>> +						USB_ENDPOINT_NUMBER_MASK;
>> +					ep_out_found = 1;
>> +				}
>> +			}
> See above how to trim down the indent here.
> [...]

-- 

--------------------------------------------------------

Dipl.-Ing. Ren? Griessl
Universit?t Bielefeld
AG Kognitronik & Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld

Telefon : +49 521-106-67362
Fax     : +49 521-106-12348
eMail   : rgriessl at cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[Marek Vasut]

On Friday, September 26, 2014 at 11:35:02 AM, Ren? Griessl wrote:
> >> changes in v3:
> >> 	-added all compatible devices from linux driver
> >> 	-fixed issues from review
> >> 
> >> changes in v2:
> >>          -added usb_ether.h to change list
> >>          -added 2nd patch to enable driver for arndale board
> > 
> > The changelog goes to the [*] marker below. And you're missing a
> > meaningful commit message too. Also, if the driver is pulled from Linux,
> > please specify from which commit in there, so future developers might
> > re-sync the driver if needed be and they'd know from which point the
> > driver was derived.
> 
> Were exactly can I find the marker?

Well, in git if you pulled the driver from Linux. Use git log path/to/file(s)

[...]

> >> +static int curr_eth_dev; /* index for name of next device detected */
> >> +
> >> +/* driver private */
> >> +struct asix_private {
> >> +	int flags;
> >> +};
> > 
> > This thing is a little iffy ... do you really need a full-blown struct
> > here or can you just use array ?
> 
> This struct is used to cast the flags to the U-Boot ueth driver. (see
> line 589 of c-file)

OK, I see assignment of the ->flags , but I don't see where this is ever used. 
Is this ->flags used at all ?

> >> +/*
> >> + * Asix infrastructure commands
> >> + */
> >> +static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
> >> index, +			     u16 size, void *data)
> >> +{
> >> +	int len;
> >> +
> >> +	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x
> >> size=%d\n", +	      cmd, value, index, size);
> >> +
> >> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
> > 
> > I think if you really enable the debug to generate a printf() , the
> > compiler will spew that you wrote code before variable declaration.
> 
> Really? I took all of the variables from the function call. So with one
> has not declaration?

You have this:

int len;

printf(...); /* this comes from the debug() if debug is enabled */

char blah.....; /* This comes from the expansion of ALLOC_CACHE_ALIGN_BUFFER()*/

See include/common.h for the ALLOC_CACHE_ALIGN_BUFFER() and debug() macro 
definition .

[...]

> >> +	if (link_detected) {
> >> +		if (timeout != 0)
> >> +			printf("done.\n");
> >> +			return 0;
> > 
> > Where does this return 0; belong to ?
> 
> it quits the init function, because a link is detected. Is it more
> likely to put a goto here?

It's the alignment that is confusing. Do you exit only if (!timeout) is true or 
do you exit unconditionally if (link_detected) ?

> >> +	} else {/*reset device and try again*/
> >> +		printf("unable to connect.\n");
> >> +		printf("Reset Ethernet Device\n");
> >> +		asix_basic_reset(dev);
> >> +		timeout = 0;
> >> +		do {
> >> +			asix_read_cmd(dev, AX_ACCESS_PHY, 0x03,
> >> +				      MII_BMSR, 2, buf);
> >> +			link_detected = *tmp16 & BMSR_LSTATUS;
> >> +			if (!link_detected) {
> >> +				if (timeout == 0)
> >> +					printf("Waiting for Ethernet
> > 
> > connection... ");
> > 
> >> +				udelay(TIMEOUT_RESOLUTION * 1000);
> > 
> > mdelay()
> > 
> >> +				timeout += TIMEOUT_RESOLUTION;
> >> +			}
> >> +		} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);
> >> +		if (link_detected) {
> >> +			if (timeout != 0)
> >> +				printf("done.\n");
> >> +				return 0;
> >> +			} else {
> >> +				printf("unable to connect.\n");
> >> +				goto out_err;
> >> +				}
> > 
> > The indent is crazy in here ...
> 
> I will put the link detect routine in a separate function.

That's OK, but the indent could also use some work ...

Thanks!

[...]

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[René Griessl]

> On Friday, September 26, 2014 at 11:35:02 AM, Ren? Griessl wrote:
>>>> changes in v3:
>>>> 	-added all compatible devices from linux driver
>>>> 	-fixed issues from review
>>>>
>>>> changes in v2:
>>>>           -added usb_ether.h to change list
>>>>           -added 2nd patch to enable driver for arndale board
>>> The changelog goes to the [*] marker below. And you're missing a
>>> meaningful commit message too. Also, if the driver is pulled from Linux,
>>> please specify from which commit in there, so future developers might
>>> re-sync the driver if needed be and they'd know from which point the
>>> driver was derived.
>> Were exactly can I find the marker?
> Well, in git if you pulled the driver from Linux. Use git log path/to/file(s)
With git log I can see the commit messages, right?
Does that mean, that I have omit the change-log in the commit message, 
or only write the
last changes in there?
> [...]
>
>>>> +static int curr_eth_dev; /* index for name of next device detected */
>>>> +
>>>> +/* driver private */
>>>> +struct asix_private {
>>>> +	int flags;
>>>> +};
>>> This thing is a little iffy ... do you really need a full-blown struct
>>> here or can you just use array ?
>> This struct is used to cast the flags to the U-Boot ueth driver. (see
>> line 589 of c-file)
> OK, I see assignment of the ->flags , but I don't see where this is ever used.
> Is this ->flags used at all ?
Well thats correct. In the asix.c driver the flags are used to handle 
small differences between
the devices. This is left inside for future work, if it becomes 
necessary to handle things different.
>>>> +/*
>>>> + * Asix infrastructure commands
>>>> + */
>>>> +static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value, u16
>>>> index, +			     u16 size, void *data)
>>>> +{
>>>> +	int len;
>>>> +
>>>> +	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x
>>>> size=%d\n", +	      cmd, value, index, size);
>>>> +
>>>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
>>> I think if you really enable the debug to generate a printf() , the
>>> compiler will spew that you wrote code before variable declaration.
>> Really? I took all of the variables from the function call. So with one
>> has not declaration?
> You have this:
>
> int len;
>
> printf(...); /* this comes from the debug() if debug is enabled */
>
> char blah.....; /* This comes from the expansion of ALLOC_CACHE_ALIGN_BUFFER()*/
>
> See include/common.h for the ALLOC_CACHE_ALIGN_BUFFER() and debug() macro
> definition .
OK, now I see. Then I have a variable definition after the printf.
> [...]
>
>>>> +	if (link_detected) {
>>>> +		if (timeout != 0)
>>>> +			printf("done.\n");
>>>> +			return 0;
>>> Where does this return 0; belong to ?
>> it quits the init function, because a link is detected. Is it more
>> likely to put a goto here?
> It's the alignment that is confusing. Do you exit only if (!timeout) is true or
> do you exit unconditionally if (link_detected) ?
I changed the name of the variable to time_waited and the check is now 
(time_waited > 0)
so done is only printed when you really had to wait for the connection. 
If the connection
was already established the messages will not be printed.
And the return has one tab less now.
> [...]

-- 

--------------------------------------------------------

Dipl.-Ing. Ren? Griessl
Universit?t Bielefeld
AG Kognitronik & Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld

Telefon : +49 521-106-67362
Fax     : +49 521-106-12348
eMail   : rgriessl at cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[Marek Vasut]

On Friday, September 26, 2014 at 05:42:10 PM, Ren? Griessl wrote:
> > On Friday, September 26, 2014 at 11:35:02 AM, Ren? Griessl wrote:
> >>>> changes in v3:
> >>>> 	-added all compatible devices from linux driver
> >>>> 	-fixed issues from review
> >>>> 
> >>>> changes in v2:
> >>>>           -added usb_ether.h to change list
> >>>>           -added 2nd patch to enable driver for arndale board
> >>> 
> >>> The changelog goes to the [*] marker below. And you're missing a
> >>> meaningful commit message too. Also, if the driver is pulled from
> >>> Linux, please specify from which commit in there, so future developers
> >>> might re-sync the driver if needed be and they'd know from which point
> >>> the driver was derived.
> >> 
> >> Were exactly can I find the marker?
> > 
> > Well, in git if you pulled the driver from Linux. Use git log
> > path/to/file(s)
> 
> With git log I can see the commit messages, right?
> Does that mean, that I have omit the change-log in the commit message,
> or only write the
> last changes in there?

The important part is the Commit ID there.

> > [...]
> > 
> >>>> +static int curr_eth_dev; /* index for name of next device detected */
> >>>> +
> >>>> +/* driver private */
> >>>> +struct asix_private {
> >>>> +	int flags;
> >>>> +};
> >>> 
> >>> This thing is a little iffy ... do you really need a full-blown struct
> >>> here or can you just use array ?
> >> 
> >> This struct is used to cast the flags to the U-Boot ueth driver. (see
> >> line 589 of c-file)
> > 
> > OK, I see assignment of the ->flags , but I don't see where this is ever
> > used. Is this ->flags used at all ?
> 
> Well thats correct. In the asix.c driver the flags are used to handle
> small differences between
> the devices. This is left inside for future work, if it becomes
> necessary to handle things different.

Zap them, it's dead code.

> >>>> +/*
> >>>> + * Asix infrastructure commands
> >>>> + */
> >>>> +static int asix_write_cmd(struct ueth_data *dev, u8 cmd, u16 value,
> >>>> u16 index, +			     u16 size, void *data)
> >>>> +{
> >>>> +	int len;
> >>>> +
> >>>> +	debug("asix_write_cmd() cmd=0x%02x value=0x%04x index=0x%04x
> >>>> size=%d\n", +	      cmd, value, index, size);
> >>>> +
> >>>> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, size);
> >>> 
> >>> I think if you really enable the debug to generate a printf() , the
> >>> compiler will spew that you wrote code before variable declaration.
> >> 
> >> Really? I took all of the variables from the function call. So with one
> >> has not declaration?
> > 
> > You have this:
> > 
> > int len;
> > 
> > printf(...); /* this comes from the debug() if debug is enabled */
> > 
> > char blah.....; /* This comes from the expansion of
> > ALLOC_CACHE_ALIGN_BUFFER()*/
> > 
> > See include/common.h for the ALLOC_CACHE_ALIGN_BUFFER() and debug() macro
> > definition .
> 
> OK, now I see. Then I have a variable definition after the printf.

Yes.

> > [...]
> > 
> >>>> +	if (link_detected) {
> >>>> +		if (timeout != 0)
> >>>> +			printf("done.\n");
> >>>> +			return 0;
> >>> 
> >>> Where does this return 0; belong to ?
> >> 
> >> it quits the init function, because a link is detected. Is it more
> >> likely to put a goto here?
> > 
> > It's the alignment that is confusing. Do you exit only if (!timeout) is
> > true or do you exit unconditionally if (link_detected) ?
> 
> I changed the name of the variable to time_waited and the check is now
> (time_waited > 0)

Timeout was OK, there is no need to make the code diverge more.

> so done is only printed when you really had to wait for the connection.
> If the connection
> was already established the messages will not be printed.
> And the return has one tab less now.

OK, so the return happens unconditionally. That's what I wanted to know.

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[Andy Pont]

Hello Rene,

> Subject: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
> 
> changes in v3:
> 	-added all compatible devices from linux driver
> 	-fixed issues from review
> 
> changes in v2:
>         -added usb_ether.h to change list
>         -added 2nd patch to enable driver for arndale board

Following the additional comments that came from Marek do you know when you
will be submitted a v4 of this patch?

Thanks for your effort on this.

Regards,

Andy.

[U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER

[René Griessl]

Hello Andy,

  today I'm back from holiday.
There is a lot of different work waiting for me, so I think it will be 
submitted by the end of next week.

Did you try the V3 Patch?

br
   Ren?

Am 06.10.2014 19:45, schrieb Andy Pont:
> Hello Rene,
>
>> Subject: [U-Boot] [PATCH V3 2/3] usb: eth: add ASIX AX88179 DRIVER
>>
>> changes in v3:
>> 	-added all compatible devices from linux driver
>> 	-fixed issues from review
>>
>> changes in v2:
>>          -added usb_ether.h to change list
>>          -added 2nd patch to enable driver for arndale board
> Following the additional comments that came from Marek do you know when you
> will be submitted a v4 of this patch?
>
> Thanks for your effort on this.
>
> Regards,
>
> Andy.
>

-- 

--------------------------------------------------------

Dipl.-Ing. Ren? Griessl
Universit?t Bielefeld
AG Kognitronik & Sensorik
Exzellenzcluster Cognitive Interaction Technology (CITEC)
Inspiration 1 (Zehlendorfer Damm 199)
33615 Bielefeld

Telefon : +49 521-106-67362
Fax     : +49 521-106-12348
eMail   : rgriessl at cit-ec.uni-bielefeld.de
Internet: http://www.ks.cit-ec.uni-bielefeld.de/