All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
@ 2011-04-13  0:46 Simon Glass
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet Simon Glass
                   ` (4 more replies)
  0 siblings, 5 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13  0:46 UTC (permalink / raw)
  To: u-boot

The SMSC95XX is a USB hub with a built-in Ethernet adapter. This adds support
for this, using the USB host network framework.

TEST=usb start; bootp; tftp ...

Signed-off-by: Simon Glass <sjg@chromium.org>

---
Changes for v2:
   - Coding style cleanup
   - Changed some comments as suggested
   - eth_set_hwaddr -> eth_write_hwaddr
   - tided up other users of eth_getenv_enetaddr_by_index()

---
 drivers/usb/eth/Makefile    |    1 +
 drivers/usb/eth/smsc95xx.c  |  922 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/eth/usb_ether.c |    7 +
 3 files changed, 930 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/eth/smsc95xx.c

diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
index 6a5f25a..e28793d 100644
--- a/drivers/usb/eth/Makefile
+++ b/drivers/usb/eth/Makefile
@@ -28,6 +28,7 @@ COBJS-$(CONFIG_USB_HOST_ETHER) += usb_ether.o
 ifdef CONFIG_USB_ETHER_ASIX
 COBJS-y += asix.o
 endif
+COBJS-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o
 
 COBJS	:= $(COBJS-y)
 SRCS	:= $(COBJS:.o=.c)
diff --git a/drivers/usb/eth/smsc95xx.c b/drivers/usb/eth/smsc95xx.c
new file mode 100644
index 0000000..fd8661f
--- /dev/null
+++ b/drivers/usb/eth/smsc95xx.c
@@ -0,0 +1,922 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * Copyright (C) 2009 NVIDIA, Corporation
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <usb.h>
+#include <linux/mii.h>
+#include "usb_ether.h"
+
+/* SMSC LAN95xx based USB 2.0 Ethernet Devices */
+
+/* Tx command words */
+#define TX_CMD_A_FIRST_SEG_		(0x00002000)
+#define TX_CMD_A_LAST_SEG_		(0x00001000)
+
+/* Rx status word */
+#define RX_STS_FL_			(0x3FFF0000)	/* Frame Length */
+#define RX_STS_ES_			(0x00008000)	/* Error Summary */
+
+/* SCSRs */
+#define ID_REV				(0x00)
+
+#define INT_STS				(0x08)
+
+#define TX_CFG				(0x10)
+#define TX_CFG_ON_			(0x00000004)
+
+#define HW_CFG				(0x14)
+#define HW_CFG_BIR_			(0x00001000)
+#define HW_CFG_RXDOFF_			(0x00000600)
+#define HW_CFG_MEF_			(0x00000020)
+#define HW_CFG_BCE_			(0x00000002)
+#define HW_CFG_LRST_			(0x00000008)
+
+#define PM_CTRL				(0x20)
+#define PM_CTL_PHY_RST_			(0x00000010)
+
+#define AFC_CFG				(0x2C)
+
+/*
+ * Hi watermark = 15.5Kb (~10 mtu pkts)
+ * low watermark = 3k (~2 mtu pkts)
+ * backpressure duration = ~ 350us
+ * Apply FC on any frame.
+ */
+#define AFC_CFG_DEFAULT			(0x00F830A1)
+
+#define E2P_CMD				(0x30)
+#define E2P_CMD_BUSY_			(0x80000000)
+#define E2P_CMD_READ_			(0x00000000)
+#define E2P_CMD_TIMEOUT_		(0x00000400)
+#define E2P_CMD_LOADED_			(0x00000200)
+#define E2P_CMD_ADDR_			(0x000001FF)
+
+#define E2P_DATA			(0x34)
+
+#define BURST_CAP			(0x38)
+
+#define INT_EP_CTL			(0x68)
+#define INT_EP_CTL_PHY_INT_		(0x00008000)
+
+#define BULK_IN_DLY			(0x6C)
+
+/* MAC CSRs */
+#define MAC_CR				(0x100)
+#define MAC_CR_MCPAS_			(0x00080000)
+#define MAC_CR_PRMS_			(0x00040000)
+#define MAC_CR_HPFILT_			(0x00002000)
+#define MAC_CR_TXEN_			(0x00000008)
+#define MAC_CR_RXEN_			(0x00000004)
+
+#define ADDRH				(0x104)
+
+#define ADDRL				(0x108)
+
+#define MII_ADDR			(0x114)
+#define MII_WRITE_			(0x02)
+#define MII_BUSY_			(0x01)
+#define MII_READ_			(0x00) /* ~of MII Write bit */
+
+#define MII_DATA			(0x118)
+
+#define FLOW				(0x11C)
+
+#define VLAN1				(0x120)
+
+#define COE_CR				(0x130)
+#define Tx_COE_EN_			(0x00010000)
+#define Rx_COE_EN_			(0x00000001)
+
+/* Vendor-specific PHY Definitions */
+#define PHY_INT_SRC			(29)
+
+#define PHY_INT_MASK			(30)
+#define PHY_INT_MASK_ANEG_COMP_		((u16)0x0040)
+#define PHY_INT_MASK_LINK_DOWN_		((u16)0x0010)
+#define PHY_INT_MASK_DEFAULT_		(PHY_INT_MASK_ANEG_COMP_ | \
+					 PHY_INT_MASK_LINK_DOWN_)
+
+/* USB Vendor Requests */
+#define USB_VENDOR_REQUEST_WRITE_REGISTER	0xA0
+#define USB_VENDOR_REQUEST_READ_REGISTER	0xA1
+
+/* Some extra defines */
+#define HS_USB_PKT_SIZE			(512)
+#define FS_USB_PKT_SIZE			(64)
+#define DEFAULT_HS_BURST_CAP_SIZE	(16 * 1024 + 5 * HS_USB_PKT_SIZE)
+#define DEFAULT_FS_BURST_CAP_SIZE	(6 * 1024 + 33 * FS_USB_PKT_SIZE)
+#define DEFAULT_BULK_IN_DELAY		(0x00002000)
+#define MAX_SINGLE_PACKET_SIZE		(2048)
+#define EEPROM_MAC_OFFSET		(0x01)
+#define SMSC95XX_INTERNAL_PHY_ID	(1)
+#define ETH_P_8021Q	0x8100          /* 802.1Q VLAN Extended Header  */
+
+/* local defines */
+#define SMSC95XX_BASE_NAME "sms"
+#define USB_CTRL_SET_TIMEOUT 5000
+#define USB_CTRL_GET_TIMEOUT 5000
+#define USB_BULK_SEND_TIMEOUT 5000
+#define USB_BULK_RECV_TIMEOUT 5000
+
+#define AX_RX_URB_SIZE 2048
+#define PHY_CONNECT_TIMEOUT 5000
+
+/* local vars */
+static int curr_eth_dev; /* index for name of next device detected */
+static int turbo_mode = 1;
+
+
+/*
+ * Smsc95xx infrastructure commands
+ */
+static int smsc95xx_write_reg(struct ueth_data *dev, u32 index, u32 data)
+{
+	int len;
+
+	cpu_to_le32s(&data);
+
+	len = usb_control_msg(dev->pusb_dev, usb_sndctrlpipe(dev->pusb_dev, 0),
+		USB_VENDOR_REQUEST_WRITE_REGISTER,
+		USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		00, index, &data, sizeof(data), USB_CTRL_SET_TIMEOUT);
+
+	return len == sizeof(data) ? 0 : -1;
+}
+
+static int smsc95xx_read_reg(struct ueth_data *dev, u32 index, u32 *data)
+{
+	int len;
+
+	len = usb_control_msg(dev->pusb_dev, usb_rcvctrlpipe(dev->pusb_dev, 0),
+		USB_VENDOR_REQUEST_READ_REGISTER,
+		USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		00, index, data, sizeof(data), USB_CTRL_GET_TIMEOUT);
+
+	le32_to_cpus(data);
+	return len == sizeof(data) ? 0 : -1;
+}
+
+/* Loop until the read is completed with timeout */
+static int smsc95xx_phy_wait_not_busy(struct ueth_data *dev)
+{
+	unsigned long start_time = get_timer(0);
+	u32 val;
+
+	do {
+		smsc95xx_read_reg(dev, MII_ADDR, &val);
+		if (!(val & MII_BUSY_))
+			return 0;
+	} while (get_timer(start_time) < 1 * 1000 * 1000);
+
+	return -1;
+}
+
+static int smsc95xx_mdio_read(struct ueth_data *dev, int phy_id, int idx)
+{
+	u32 val, addr;
+
+	/* confirm MII not busy */
+	if (smsc95xx_phy_wait_not_busy(dev)) {
+		debug("MII is busy in smsc95xx_mdio_read\n");
+		return -1;
+	}
+
+	/* set the address, index & direction (read from PHY) */
+	addr = (phy_id << 11) | (idx << 6) | MII_READ_;
+	smsc95xx_write_reg(dev, MII_ADDR, addr);
+
+	if (smsc95xx_phy_wait_not_busy(dev)) {
+		debug("Timed out reading MII reg %02X\n", idx);
+		return -1;
+	}
+
+	smsc95xx_read_reg(dev, MII_DATA, &val);
+
+	return (u16)(val & 0xFFFF);
+}
+
+static void smsc95xx_mdio_write(struct ueth_data *dev, int phy_id, int idx,
+				int regval)
+{
+	u32 val, addr;
+
+	/* confirm MII not busy */
+	if (smsc95xx_phy_wait_not_busy(dev)) {
+		debug("MII is busy in smsc95xx_mdio_write\n");
+		return;
+	}
+
+	val = regval;
+	smsc95xx_write_reg(dev, MII_DATA, val);
+
+	/* set the address, index & direction (write to PHY) */
+	addr = (phy_id << 11) | (idx << 6) | MII_WRITE_;
+	smsc95xx_write_reg(dev, MII_ADDR, addr);
+
+	if (smsc95xx_phy_wait_not_busy(dev))
+		debug("Timed out writing MII reg %02X\n", idx);
+}
+
+static int smsc95xx_eeprom_confirm_not_busy(struct ueth_data *dev)
+{
+	unsigned long start_time = get_timer(0);
+	u32 val;
+
+	do {
+		smsc95xx_read_reg(dev, E2P_CMD, &val);
+		if (!(val & E2P_CMD_LOADED_)) {
+			debug("No EEPROM present\n");
+			return -1;
+		}
+		if (!(val & E2P_CMD_BUSY_))
+			return 0;
+		udelay(40);
+	} while (get_timer(start_time) < 1 * 1000 * 1000);
+
+	debug("EEPROM is busy\n");
+	return -1;
+}
+
+static int smsc95xx_wait_eeprom(struct ueth_data *dev)
+{
+	unsigned long start_time = get_timer(0);
+	u32 val;
+
+	do {
+		smsc95xx_read_reg(dev, E2P_CMD, &val);
+		if (!(val & E2P_CMD_BUSY_) || (val & E2P_CMD_TIMEOUT_))
+			break;
+		udelay(40);
+	} while (get_timer(start_time) < 1 * 1000 * 1000);
+
+	if (val & (E2P_CMD_TIMEOUT_ | E2P_CMD_BUSY_)) {
+		debug("EEPROM read operation timeout\n");
+		return -1;
+	}
+	return 0;
+}
+
+static int smsc95xx_read_eeprom(struct ueth_data *dev, u32 offset, u32 length,
+				u8 *data)
+{
+	u32 val;
+	int i, ret;
+
+	ret = smsc95xx_eeprom_confirm_not_busy(dev);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < length; i++) {
+		val = E2P_CMD_BUSY_ | E2P_CMD_READ_ | (offset & E2P_CMD_ADDR_);
+		smsc95xx_write_reg(dev, E2P_CMD, val);
+
+		ret = smsc95xx_wait_eeprom(dev);
+		if (ret < 0)
+			return ret;
+
+		smsc95xx_read_reg(dev, E2P_DATA, &val);
+		data[i] = val & 0xFF;
+		offset++;
+	}
+	return 0;
+}
+
+/*
+ * mii_nway_restart - restart NWay (autonegotiation) for this interface
+ *
+ * Returns 0 on success, negative on error.
+ */
+static int mii_nway_restart(struct ueth_data *dev)
+{
+	int bmcr;
+	int r = -1;
+
+	/* if autoneg is off, it's an error */
+	bmcr = smsc95xx_mdio_read(dev, dev->phy_id, MII_BMCR);
+
+	if (bmcr & BMCR_ANENABLE) {
+		bmcr |= BMCR_ANRESTART;
+		smsc95xx_mdio_write(dev, dev->phy_id, MII_BMCR, bmcr);
+		r = 0;
+	}
+	return r;
+}
+
+static int smsc95xx_phy_initialize(struct ueth_data *dev)
+{
+	smsc95xx_mdio_write(dev, dev->phy_id, MII_BMCR, BMCR_RESET);
+	smsc95xx_mdio_write(dev, dev->phy_id, MII_ADVERTISE,
+		ADVERTISE_ALL | ADVERTISE_CSMA | ADVERTISE_PAUSE_CAP |
+		ADVERTISE_PAUSE_ASYM);
+
+	/* read to clear */
+	smsc95xx_mdio_read(dev, dev->phy_id, PHY_INT_SRC);
+
+	smsc95xx_mdio_write(dev, dev->phy_id, PHY_INT_MASK,
+		PHY_INT_MASK_DEFAULT_);
+	mii_nway_restart(dev);
+
+	debug("phy initialised succesfully\n");
+	return 0;
+}
+
+static int smsc95xx_init_mac_address(struct eth_device *eth,
+		struct ueth_data *dev)
+{
+	/* try reading mac address from EEPROM */
+	if (smsc95xx_read_eeprom(dev, EEPROM_MAC_OFFSET, ETH_ALEN,
+			eth->enetaddr) == 0) {
+		if (is_valid_ether_addr(eth->enetaddr)) {
+			/* eeprom values are valid so use them */
+			debug("MAC address read from EEPROM\n");
+			return 0;
+		}
+	}
+
+	/*
+	 * No eeprom, or eeprom values are invalid. Generating a random MAC
+	 * address is not safe. Just return an error.
+	 */
+	return -1;
+}
+
+static int smsc95xx_write_hwaddr(struct eth_device *eth)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	u32 addr_lo, addr_hi;
+	int ret;
+
+	/* set hardware address */
+	debug("** %s()\n", __func__);
+	addr_lo = cpu_to_le32(*((u32 *)eth->enetaddr));
+	addr_hi = cpu_to_le16(*((u16 *)(eth->enetaddr + 4)));
+	ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
+	if (ret < 0) {
+		debug("Failed to write ADDRL: %d\n", ret);
+		return ret;
+	}
+
+	ret = smsc95xx_write_reg(dev, ADDRH, addr_hi);
+	if (ret < 0) {
+		debug("Failed to write ADDRH: %d\n", ret);
+		return ret;
+	}
+	debug("MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
+		eth->enetaddr[0], eth->enetaddr[1],
+		eth->enetaddr[2], eth->enetaddr[3],
+		eth->enetaddr[4], eth->enetaddr[5]);
+	dev->have_hwaddr = 1;
+	return 0;
+}
+
+/* Enable or disable Tx & Rx checksum offload engines */
+static int smsc95xx_set_csums(struct ueth_data *dev,
+		int use_tx_csum, int use_rx_csum)
+{
+	u32 read_buf;
+	int ret = smsc95xx_read_reg(dev, COE_CR, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read COE_CR: %d\n", ret);
+		return ret;
+	}
+
+	if (use_tx_csum)
+		read_buf |= Tx_COE_EN_;
+	else
+		read_buf &= ~Tx_COE_EN_;
+
+	if (use_rx_csum)
+		read_buf |= Rx_COE_EN_;
+	else
+		read_buf &= ~Rx_COE_EN_;
+
+	ret = smsc95xx_write_reg(dev, COE_CR, read_buf);
+	if (ret < 0) {
+		debug("Failed to write COE_CR: %d\n", ret);
+		return ret;
+	}
+
+	debug("COE_CR = 0x%08x\n", read_buf);
+	return 0;
+}
+
+static void smsc95xx_set_multicast(struct ueth_data *dev)
+{
+	/* No multicast in u-boot */
+	dev->mac_cr &= ~(MAC_CR_PRMS_ | MAC_CR_MCPAS_ | MAC_CR_HPFILT_);
+}
+
+/* starts the TX path */
+static void smsc95xx_start_tx_path(struct ueth_data *dev)
+{
+	u32 reg_val;
+
+	/* Enable Tx at MAC */
+	dev->mac_cr |= MAC_CR_TXEN_;
+
+	smsc95xx_write_reg(dev, MAC_CR, dev->mac_cr);
+
+	/* Enable Tx at SCSRs */
+	reg_val = TX_CFG_ON_;
+	smsc95xx_write_reg(dev, TX_CFG, reg_val);
+}
+
+/* Starts the Receive path */
+static void smsc95xx_start_rx_path(struct ueth_data *dev)
+{
+	dev->mac_cr |= MAC_CR_RXEN_;
+	smsc95xx_write_reg(dev, MAC_CR, dev->mac_cr);
+}
+
+/*
+ * Smsc95xx callbacks
+ */
+static int smsc95xx_init(struct eth_device *eth, bd_t *bd)
+{
+	int ret;
+	u32 write_buf;
+	u32 read_buf;
+	u32 burst_cap;
+	int timeout;
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+#define TIMEOUT_RESOLUTION 50	/* ms */
+	int link_detected;
+
+	debug("** %s()\n", __func__);
+	dev->phy_id = SMSC95XX_INTERNAL_PHY_ID; /* fixed phy id */
+
+	write_buf = HW_CFG_LRST_;
+	ret = smsc95xx_write_reg(dev, HW_CFG, write_buf);
+	if (ret < 0) {
+		debug("Failed to write HW_CFG_LRST_ bit in HW_CFG "
+			"register, ret = %d\n", ret);
+		return ret;
+	}
+
+	timeout = 0;
+	do {
+		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
+		if (ret < 0) {
+			debug("Failed to read HW_CFG: %d\n", ret);
+			return ret;
+		}
+		udelay(10 * 1000);
+		timeout++;
+	} while ((read_buf & HW_CFG_LRST_) && (timeout < 100));
+
+	if (timeout >= 100) {
+		debug("timeout waiting for completion of Lite Reset\n");
+		return -1;
+	}
+
+	write_buf = PM_CTL_PHY_RST_;
+	ret = smsc95xx_write_reg(dev, PM_CTRL, write_buf);
+	if (ret < 0) {
+		debug("Failed to write PM_CTRL: %d\n", ret);
+		return ret;
+	}
+
+	timeout = 0;
+	do {
+		ret = smsc95xx_read_reg(dev, PM_CTRL, &read_buf);
+		if (ret < 0) {
+			debug("Failed to read PM_CTRL: %d\n", ret);
+			return ret;
+		}
+		udelay(10 * 1000);
+		timeout++;
+	} while ((read_buf & PM_CTL_PHY_RST_) && (timeout < 100));
+	if (timeout >= 100) {
+		debug("timeout waiting for PHY Reset\n");
+		return -1;
+	}
+	if (!dev->have_hwaddr && smsc95xx_init_mac_address(eth, dev) == 0)
+		dev->have_hwaddr = 1;
+	if (!dev->have_hwaddr) {
+		puts("Error: SMSC95xx: No MAC address set - set usbethaddr\n");
+		return -1;
+	}
+	if (smsc95xx_write_hwaddr(eth) < 0)
+		return -1;
+
+	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read HW_CFG: %d\n", ret);
+		return ret;
+	}
+	debug("Read Value from HW_CFG : 0x%08x\n", read_buf);
+
+	read_buf |= HW_CFG_BIR_;
+	ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
+	if (ret < 0) {
+		debug("Failed to write HW_CFG_BIR_ bit in HW_CFG "
+			"register, ret = %d\n", ret);
+		return ret;
+	}
+
+	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read HW_CFG: %d\n", ret);
+		return ret;
+	}
+	debug("Read Value from HW_CFG after writing "
+		"HW_CFG_BIR_: 0x%08x\n", read_buf);
+
+	if (!turbo_mode) {
+		burst_cap = 0;
+		dev->rx_urb_size = MAX_SINGLE_PACKET_SIZE;
+	} else if (dev->pusb_dev->speed == USB_SPEED_HIGH) {
+		burst_cap = DEFAULT_HS_BURST_CAP_SIZE / HS_USB_PKT_SIZE;
+		dev->rx_urb_size = DEFAULT_HS_BURST_CAP_SIZE;
+	} else {
+		burst_cap = DEFAULT_FS_BURST_CAP_SIZE / FS_USB_PKT_SIZE;
+		dev->rx_urb_size = DEFAULT_FS_BURST_CAP_SIZE;
+	}
+	debug("rx_urb_size=%ld\n", (ulong)dev->rx_urb_size);
+
+	ret = smsc95xx_write_reg(dev, BURST_CAP, burst_cap);
+	if (ret < 0) {
+		debug("Failed to write BURST_CAP: %d\n", ret);
+		return ret;
+	}
+
+	ret = smsc95xx_read_reg(dev, BURST_CAP, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read BURST_CAP: %d\n", ret);
+		return ret;
+	}
+	debug("Read Value from BURST_CAP after writing: 0x%08x\n", read_buf);
+
+	read_buf = DEFAULT_BULK_IN_DELAY;
+	ret = smsc95xx_write_reg(dev, BULK_IN_DLY, read_buf);
+	if (ret < 0) {
+		debug("ret = %d", ret);
+		return ret;
+	}
+
+	ret = smsc95xx_read_reg(dev, BULK_IN_DLY, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read BULK_IN_DLY: %d\n", ret);
+		return ret;
+	}
+	debug("Read Value from BULK_IN_DLY after writing: "
+			"0x%08x\n", read_buf);
+
+	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read HW_CFG: %d\n", ret);
+		return ret;
+	}
+	debug("Read Value from HW_CFG: 0x%08x\n", read_buf);
+
+	if (turbo_mode)
+		read_buf |= (HW_CFG_MEF_ | HW_CFG_BCE_);
+	read_buf &= ~HW_CFG_RXDOFF_;
+
+#ifdef CONFIG_TEGRA2
+	/* Tegra2 requires NET_IP_ALIGN = 0 */
+#define NET_IP_ALIGN 0
+#else
+	/* set Rx data offset=2, Make IP header aligns on word boundary. */
+#define NET_IP_ALIGN 2
+#endif
+	read_buf |= NET_IP_ALIGN << 9;
+
+	ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
+	if (ret < 0) {
+		debug("Failed to write HW_CFG register, ret=%d\n", ret);
+		return ret;
+	}
+
+	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read HW_CFG: %d\n", ret);
+		return ret;
+	}
+	debug("Read Value from HW_CFG after writing: 0x%08x\n", read_buf);
+
+	write_buf = 0xFFFFFFFF;
+	ret = smsc95xx_write_reg(dev, INT_STS, write_buf);
+	if (ret < 0) {
+		debug("Failed to write INT_STS register, ret=%d\n", ret);
+		return ret;
+	}
+
+	ret = smsc95xx_read_reg(dev, ID_REV, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read ID_REV: %d\n", ret);
+		return ret;
+	}
+	debug("ID_REV = 0x%08x\n", read_buf);
+
+	/* Init Tx */
+	write_buf = 0;
+	ret = smsc95xx_write_reg(dev, FLOW, write_buf);
+	if (ret < 0) {
+		debug("Failed to write FLOW: %d\n", ret);
+		return ret;
+	}
+
+	read_buf = AFC_CFG_DEFAULT;
+	ret = smsc95xx_write_reg(dev, AFC_CFG, read_buf);
+	if (ret < 0) {
+		debug("Failed to write AFC_CFG: %d\n", ret);
+		return ret;
+	}
+
+	ret = smsc95xx_read_reg(dev, MAC_CR, &dev->mac_cr);
+	if (ret < 0) {
+		debug("Failed to read MAC_CR: %d\n", ret);
+		return ret;
+	}
+
+	/* Init Rx. Set Vlan */
+	write_buf = (u32)ETH_P_8021Q;
+	ret = smsc95xx_write_reg(dev, VLAN1, write_buf);
+	if (ret < 0) {
+		debug("Failed to write VAN1: %d\n", ret);
+		return ret;
+	}
+
+	/* Disable checksum offload engines */
+	ret = smsc95xx_set_csums(dev, 0, 0);
+	if (ret < 0) {
+		debug("Failed to set csum offload: %d\n", ret);
+		return ret;
+	}
+	smsc95xx_set_multicast(dev);
+
+	if (smsc95xx_phy_initialize(dev) < 0)
+		return -1;
+	ret = smsc95xx_read_reg(dev, INT_EP_CTL, &read_buf);
+	if (ret < 0) {
+		debug("Failed to read INT_EP_CTL: %d", ret);
+		return ret;
+	}
+	/* enable PHY interrupts */
+	read_buf |= INT_EP_CTL_PHY_INT_;
+
+	ret = smsc95xx_write_reg(dev, INT_EP_CTL, read_buf);
+	if (ret < 0) {
+		debug("Failed to write INT_EP_CTL: %d", ret);
+		return ret;
+	}
+
+	smsc95xx_start_tx_path(dev);
+	smsc95xx_start_rx_path(dev);
+
+	timeout = 0;
+	do {
+		link_detected = smsc95xx_mdio_read(dev, dev->phy_id, MII_BMSR)
+			& 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");
+	} else {
+		printf("unable to connect.\n");
+		return -1;
+	}
+	return 0;
+}
+
+static int smsc95xx_send(struct eth_device *eth, volatile void* packet,
+			 int length)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	int err;
+	int actual_len;
+	u32 tx_cmd_a;
+	u32 tx_cmd_b;
+	unsigned char msg[PKTSIZE + sizeof(tx_cmd_a) + sizeof(tx_cmd_b)];
+
+	debug("** %s(), len %d, buf %#x\n", __func__, length, (int)msg);
+	if (length > PKTSIZE)
+		return -1;
+
+	tx_cmd_a = (u32)length | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
+	tx_cmd_b = (u32)length;
+	cpu_to_le32s(&tx_cmd_a);
+	cpu_to_le32s(&tx_cmd_b);
+
+	/* prepend cmd_a and cmd_b */
+	memcpy(msg, &tx_cmd_a, sizeof(tx_cmd_a));
+	memcpy(msg + sizeof(tx_cmd_a), &tx_cmd_b, sizeof(tx_cmd_b));
+	memcpy(msg + sizeof(tx_cmd_a) + sizeof(tx_cmd_b), (void *)packet,
+	       length);
+	err = usb_bulk_msg(dev->pusb_dev,
+				usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
+				(void *)msg,
+				length + sizeof(tx_cmd_a) + sizeof(tx_cmd_b),
+				&actual_len,
+				USB_BULK_SEND_TIMEOUT);
+	debug("Tx: len = %u, actual = %u, err = %d\n",
+	      length + sizeof(tx_cmd_a) + sizeof(tx_cmd_b),
+	      actual_len, err);
+	return err;
+}
+
+static int smsc95xx_recv(struct eth_device *eth)
+{
+	struct ueth_data *dev = (struct ueth_data *)eth->priv;
+	static unsigned char  recv_buf[AX_RX_URB_SIZE];
+	unsigned char *buf_ptr;
+	int err;
+	int actual_len;
+	u32 packet_len;
+	int cur_buf_align;
+
+	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;
+	}
+
+	buf_ptr = recv_buf;
+	while (actual_len > 0) {
+		/*
+		 * 1st 4 bytes contain the length of the actual data plus error
+		 * info. Extract data length.
+		 */
+		if (actual_len < sizeof(packet_len)) {
+			debug("Rx: incomplete packet length\n");
+			return -1;
+		}
+		memcpy(&packet_len, buf_ptr, sizeof(packet_len));
+		le32_to_cpus(&packet_len);
+		if (packet_len & RX_STS_ES_) {
+			debug("Rx: Error header=%#x", packet_len);
+			return -1;
+		}
+		packet_len = ((packet_len & RX_STS_FL_) >> 16);
+
+		if (packet_len > actual_len - sizeof(packet_len)) {
+			debug("Rx: too large packet: %d\n", packet_len);
+			return -1;
+		}
+
+		/* Notify net stack */
+		NetReceive(buf_ptr + sizeof(packet_len), packet_len - 4);
+
+		/* Adjust for next iteration */
+		actual_len -= sizeof(packet_len) + packet_len;
+		buf_ptr += sizeof(packet_len) + packet_len;
+		cur_buf_align = (int)buf_ptr - (int)recv_buf;
+
+		if (cur_buf_align & 0x03) {
+			int align = 4 - (cur_buf_align & 0x03);
+
+			actual_len -= align;
+			buf_ptr += align;
+		}
+	}
+	return err;
+}
+
+static void smsc95xx_halt(struct eth_device *eth)
+{
+	debug("** %s()\n", __func__);
+}
+
+/*
+ * SMSC probing functions
+ */
+void smsc95xx_eth_before_probe(void)
+{
+	curr_eth_dev = 0;
+}
+
+struct smsc95xx_dongle {
+	unsigned short vendor;
+	unsigned short product;
+};
+
+static const struct smsc95xx_dongle smsc95xx_dongles[] = {
+	{ 0x0424, 0xec00 },	/* LAN9512/LAN9514 Ethernet */
+	{ 0x0424, 0x9500 },	/* LAN9500 Ethernet */
+	{ 0x0000, 0x0000 }	/* END - Do not remove */
+};
+
+/* Probe to see if a new device is actually an SMSC device */
+int smsc95xx_eth_probe(struct usb_device *dev, unsigned int ifnum,
+		      struct ueth_data *ss)
+{
+	struct usb_interface *iface;
+	struct usb_interface_descriptor *iface_desc;
+	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; smsc95xx_dongles[i].vendor != 0; i++) {
+		if (dev->descriptor.idVendor == smsc95xx_dongles[i].vendor &&
+		    dev->descriptor.idProduct == smsc95xx_dongles[i].product)
+			/* Found a supported dongle */
+			break;
+	}
+	if (smsc95xx_dongles[i].vendor == 0)
+		return 0;
+
+	/* At this point, we know we've got a live one */
+	debug("\n\nUSB Ethernet device detected\n");
+	memset(ss, '\0', sizeof(struct ueth_data));
+
+	/* 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;
+
+	/*
+	 * 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 (iface->ep_desc[i].bEndpointAddress & USB_DIR_IN)
+				ss->ep_in =
+					iface->ep_desc[i].bEndpointAddress &
+					USB_ENDPOINT_NUMBER_MASK;
+			else
+				ss->ep_out =
+					iface->ep_desc[i].bEndpointAddress &
+					USB_ENDPOINT_NUMBER_MASK;
+		}
+
+		/* 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 smsc95xx_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
+				struct eth_device *eth)
+{
+	debug("** %s()\n", __func__);
+	if (!eth) {
+		debug("%s: missing parameter.\n", __func__);
+		return 0;
+	}
+	sprintf(eth->name, "%s%d", SMSC95XX_BASE_NAME, curr_eth_dev++);
+	eth->init = smsc95xx_init;
+	eth->send = smsc95xx_send;
+	eth->recv = smsc95xx_recv;
+	eth->halt = smsc95xx_halt;
+	eth->priv = ss;
+	return 1;
+}
diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
index 68a0883..7b55da3 100644
--- a/drivers/usb/eth/usb_ether.c
+++ b/drivers/usb/eth/usb_ether.c
@@ -45,6 +45,13 @@ static const struct usb_eth_prob_dev prob_dev[] = {
 		.get_info = asix_eth_get_info,
 	},
 #endif
+#ifdef CONFIG_USB_ETHER_SMSC95XX
+	{
+		.before_probe = smsc95xx_eth_before_probe,
+		.probe = smsc95xx_eth_probe,
+		.get_info = smsc95xx_eth_get_info,
+	},
+#endif
 	{ },		/* END */
 };
 
-- 
1.7.3.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13  0:46 [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Simon Glass
@ 2011-04-13  0:46 ` Simon Glass
  2011-04-13  3:48   ` Mike Frysinger
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 3/5] Add documentation for USB Host Networking Simon Glass
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2011-04-13  0:46 UTC (permalink / raw)
  To: u-boot

Built-in Ethernet adapters support setting the mac address by means of a
ethaddr environment variable for each interface (ethaddr, eth1addr, eth2addr).

This adds similar support to the USB network side, using the names
usbethaddr, usbeth1addr, etc. They are kept separate since we don't want
a USB device taking the MAC address of a built-in device or vice versa.

TEST=build, test on harmony, with setenv usbethaddr c0:c1:c0:13:0b:b8, bootp, tftp ...

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/davinci/common/misc.c |    2 +-
 drivers/net/designware.c    |    2 +-
 drivers/usb/eth/usb_ether.c |    8 ++++-
 include/net.h               |   28 ++++++++++++++++-
 net/eth.c                   |   71 ++++++++++++++++++++++++++-----------------
 5 files changed, 78 insertions(+), 33 deletions(-)

diff --git a/board/davinci/common/misc.c b/board/davinci/common/misc.c
index 2bfdf23..b2b980b 100644
--- a/board/davinci/common/misc.c
+++ b/board/davinci/common/misc.c
@@ -101,7 +101,7 @@ void davinci_sync_env_enetaddr(uint8_t *rom_enetaddr)
 {
 	uint8_t env_enetaddr[6];
 
-	eth_getenv_enetaddr_by_index(0, env_enetaddr);
+	eth_getenv_enetaddr_by_index(NULL, 0, env_enetaddr);
 	if (!memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
 		/* There is no MAC address in the environment, so we initialize
 		 * it from the value in the EEPROM. */
diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 3f5eeb7..5e0f9e3 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -500,7 +500,7 @@ int designware_initialize(u32 id, ulong base_addr, u32 phy_addr)
 	dev->iobase = (int)base_addr;
 	dev->priv = priv;
 
-	eth_getenv_enetaddr_by_index(id, &dev->enetaddr[0]);
+	eth_getenv_enetaddr_by_index(NULL, id, &dev->enetaddr[0]);
 
 	priv->dev = dev;
 	priv->mac_regs_p = (struct eth_mac_regs *)base_addr;
diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
index 7b55da3..08b491c 100644
--- a/drivers/usb/eth/usb_ether.c
+++ b/drivers/usb/eth/usb_ether.c
@@ -80,6 +80,7 @@ int is_eth_dev_on_usb_host(void)
  */
 static void probe_valid_drivers(struct usb_device *dev)
 {
+	struct eth_device *eth;
 	int j;
 
 	for (j = 0; prob_dev[j].probe && prob_dev[j].get_info; j++) {
@@ -88,9 +89,10 @@ static void probe_valid_drivers(struct usb_device *dev)
 		/*
 		 * ok, it is a supported eth device. Get info and fill it in
 		 */
+		eth = &usb_eth[usb_max_eth_dev].eth_dev;
 		if (prob_dev[j].get_info(dev,
 			&usb_eth[usb_max_eth_dev],
-			&usb_eth[usb_max_eth_dev].eth_dev)) {
+			eth)) {
 			/* found proper driver */
 			/* register with networking stack */
 			usb_max_eth_dev++;
@@ -100,7 +102,9 @@ static void probe_valid_drivers(struct usb_device *dev)
 			 * call since eth_current_changed (internally called)
 			 * relies on it
 			 */
-			eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev);
+			eth_register(eth);
+			if (eth_set_hwaddr(eth, "usbeth", usb_max_eth_dev - 1))
+				puts("Warning: failed to set MAC address\n");
 			break;
 			}
 		}
diff --git a/include/net.h b/include/net.h
index 95ef8ab..c3abf56 100644
--- a/include/net.h
+++ b/include/net.h
@@ -123,7 +123,18 @@ extern int eth_get_dev_index (void);		/* get the device index */
 extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
 extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
 extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
-extern int eth_getenv_enetaddr_by_index(int index, uchar *enetaddr);
+
+/*
+ * Get the hardware address for an ethernet interface .
+ * Args:
+ *	base_name - base name for device (NULL for "eth")
+ *	index - device index number (0 for first)
+ *	enetaddr - returns 6 byte hardware address
+ * Returns:
+ *	Return true if the address is valid.
+ */
+extern int eth_getenv_enetaddr_by_index(const char *base_name, int index,
+					uchar *enetaddr);
 
 extern int usb_eth_initialize(bd_t *bi);
 extern int eth_init(bd_t *bis);			/* Initialize the device */
@@ -136,6 +147,18 @@ extern int eth_rx(void);			/* Check for received packets */
 extern void eth_halt(void);			/* stop SCC */
 extern char *eth_get_name(void);		/* get name of current device */
 
+/*
+ * Set the hardware address for an ethernet interface based on 'eth%daddr'
+ * environment variable (or just 'ethaddr' if eth_number is 0).
+ * Args:
+ *	base_name - base name for device (NULL for "eth")
+ *	eth_number - value of %d (0 for first device of this type)
+ * Returns:
+ *	0 is success, non-zero is error status from driver.
+ */
+int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
+		     int eth_number);
+
 #ifdef CONFIG_MCAST_TFTP
 int eth_mcast_join( IPaddr_t mcast_addr, u8 join);
 u32 ether_crc (size_t len, unsigned char const *p);
@@ -332,6 +355,9 @@ extern uchar		NetOurEther[6];		/* Our ethernet address		*/
 extern uchar		NetServerEther[6];	/* Boot server enet address	*/
 extern IPaddr_t		NetOurIP;		/* Our    IP addr (0 = unknown)	*/
 extern IPaddr_t		NetServerIP;		/* Server IP addr (0 = unknown)	*/
+
+/* Tftp Server IP addr (0 = unknown)*/
+extern IPaddr_t		NetTftpServerIP;
 extern volatile uchar * NetTxPacket;		/* THE transmit packet		*/
 extern volatile uchar * NetRxPackets[PKTBUFSRX];/* Receive packets		*/
 extern volatile uchar * NetRxPacket;		/* Current receive packet	*/
diff --git a/net/eth.c b/net/eth.c
index 3a7ff50..773ad21 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -53,10 +53,13 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr)
 	return setenv(name, buf);
 }
 
-int eth_getenv_enetaddr_by_index(int index, uchar *enetaddr)
+int eth_getenv_enetaddr_by_index(const char *base_name, int index,
+				 uchar *enetaddr)
 {
 	char enetvar[32];
-	sprintf(enetvar, index ? "eth%daddr" : "ethaddr", index);
+	sprintf(enetvar, index ? "%s%daddr" : "%saddr",
+		base_name ? base_name : "eth",
+		index);
 	return eth_getenv_enetaddr(enetvar, enetaddr);
 }
 
@@ -187,6 +190,37 @@ static void eth_current_changed(void)
 #endif
 }
 
+int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
+		   int eth_number)
+{
+	unsigned char env_enetaddr[6];
+	int ret = 0;
+
+	eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);
+
+	if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
+		if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
+			memcmp(dev->enetaddr, env_enetaddr, 6)) {
+			printf("\nWarning: %s MAC addresses don't match:\n",
+				dev->name);
+			printf("Address in SROM is         %pM\n",
+				dev->enetaddr);
+			printf("Address in environment is  %pM\n",
+				env_enetaddr);
+		}
+
+		memcpy(dev->enetaddr, env_enetaddr, 6);
+	}
+
+	if (dev->write_hwaddr &&
+		!eth_mac_skip(eth_number) &&
+		is_valid_ether_addr(dev->enetaddr)) {
+		ret = dev->write_hwaddr(dev);
+	}
+
+	return ret;
+}
+
 int eth_register(struct eth_device *dev)
 {
 	struct eth_device *d;
@@ -207,7 +241,6 @@ int eth_register(struct eth_device *dev)
 
 int eth_initialize(bd_t *bis)
 {
-	unsigned char env_enetaddr[6];
 	int eth_number = 0;
 
 	eth_devices = NULL;
@@ -254,31 +287,12 @@ int eth_initialize(bd_t *bis)
 				eth_current = dev;
 				puts (" [PRIME]");
 			}
-
 			if (strchr(dev->name, ' '))
-				puts("\nWarning: eth device name has a space!\n");
-
-			eth_getenv_enetaddr_by_index(eth_number, env_enetaddr);
-
-			if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
-				if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
-				    memcmp(dev->enetaddr, env_enetaddr, 6))
-				{
-					printf ("\nWarning: %s MAC addresses don't match:\n",
-						dev->name);
-					printf ("Address in SROM is         %pM\n",
-						dev->enetaddr);
-					printf ("Address in environment is  %pM\n",
-						env_enetaddr);
-				}
-
-				memcpy(dev->enetaddr, env_enetaddr, 6);
-			}
-			if (dev->write_hwaddr &&
-				!eth_mac_skip(eth_number) &&
-				is_valid_ether_addr(dev->enetaddr)) {
-				dev->write_hwaddr(dev);
-			}
+				puts("\nWarning: eth device name has a "
+					"space!\n");
+
+			if (eth_write_hwaddr(dev, NULL, eth_number))
+				puts("Warning: failed to set MAC address\n");
 
 			eth_number++;
 			dev = dev->next;
@@ -353,7 +367,8 @@ int eth_init(bd_t *bis)
 	do {
 		uchar env_enetaddr[6];
 
-		if (eth_getenv_enetaddr_by_index(eth_number, env_enetaddr))
+		if (eth_getenv_enetaddr_by_index(NULL, eth_number,
+						 env_enetaddr))
 			memcpy(dev->enetaddr, env_enetaddr, 6);
 
 		++eth_number;
-- 
1.7.3.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 3/5] Add documentation for USB Host Networking
  2011-04-13  0:46 [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Simon Glass
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet Simon Glass
@ 2011-04-13  0:46 ` Simon Glass
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 4/5] Put common autoload code into AutoLoad() function Simon Glass
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13  0:46 UTC (permalink / raw)
  To: u-boot

This describes what it is for, devices supported, how to enable for your
board in U-Boot, setting up the server, and notes about MAC addresses.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 doc/README.usb |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 162 insertions(+), 1 deletions(-)

diff --git a/doc/README.usb b/doc/README.usb
index 9aa4f62..8693a05 100644
--- a/doc/README.usb
+++ b/doc/README.usb
@@ -79,4 +79,165 @@ CONFIG_USB_UHCI	    defines the lowlevel part.A lowlevel part must be defined
 		    if using CONFIG_CMD_USB
 CONFIG_USB_KEYBOARD enables the USB Keyboard
 CONFIG_USB_STORAGE  enables the USB storage devices
-CONFIG_USB_HOST_ETHER	enables USB ethernet dongle support
+CONFIG_USB_HOST_ETHER	enables USB ethernet adapter support
+
+
+USB Host Networking
+===================
+
+If you have a supported USB Ethernet adapter you can use it in U-Boot
+to obtain an IP address and load a kernel from a network server.
+
+USB Host Networking is different from making your board act as a USB
+client. In that case your board is pretending to be an Ethernet adapter
+and will appear as a network interface to an attached computer. The
+connection is via a USB cable with the computer acting as the host.
+
+With USB Host Networking, your board is the USB host. It controls the
+Ethernet adapter to which it is directly connected and the connection to
+the outside world is your adapter's Ethernet cable. Your board becomes a
+first class network device, able to connect and perform network
+operations independently of your computer.
+
+
+Device support
+--------------
+
+Currently supported devices are listed in the drivers according to
+their vendor and product IDs. You can check your device by connecting it
+to a Linux machine and typing 'lsusb'. The drivers are in
+drivers/usb/eth.
+
+For example this lsusb output line shows a device with Vendor ID 0x0x95
+and product ID 0x7720:
+
+Bus 002 Device 010: ID 0b95:7720 ASIX Electronics Corp. AX88772
+
+If you look at drivers/usb/eth/asix.c you will see this line within the
+supported device list, so we know this adapter is supported.
+
+        { 0x0b95, 0x7720 },     /* Trendnet TU2-ET100 V3.0R */
+
+If your adapter is not listed there is a still a chance that it will
+work. Try looking up the manufacturer of the chip inside your adapter.
+or take the adapter apart and look for chip markings. Then add a line
+for your vendor/product ID into the table of the appropriate driver,
+build U-Boot and see if it works. If not then there might be differences
+between the chip in your adapter and the driver. You could try to get a
+datasheet for your device and add support for it to U-Boot. This is not
+particularly difficult - you only need to provide support for four basic
+functions: init, halt, send and recv.
+
+
+Enabling USB Host Networking
+----------------------------
+
+The normal U-Boot commands are used with USB networking, but you must
+start USB first. For example:
+
+usb start
+setenv bootfile /tftpboot/uImage
+setenv autoload y
+bootp
+
+
+(The autoload option makes bootp automatically load the boot file.)
+
+To enable USB Host Ethernet in U-Boot, your platform must of course
+support USB with CONFIG_CMD_USB enabled and working. You will need to
+add some config settings to your board header file:
+
+#define CONFIG_USB_HOST_ETHER   /* Enable USB Ethernet adapters */
+#define CONFIG_USB_ETHER_ASIX   /* Asix, or whatever driver(s) you want */
+
+You will also want to enable the network commands:
+
+#define CONFIG_CMD_NET
+#define CONFIG_NET_MULTI
+#define CONFIG_CMD_PING
+#define CONFIG_CMD_DHCP
+
+and some bootp options, which tell your board to obtain its subnet,
+gateway IP, host name and boot path from the bootp/dhcp server:
+
+#define CONFIG_BOOTP_SUBNETMASK
+#define CONFIG_BOOTP_GATEWAY
+#define CONFIG_BOOTP_HOSTNAME
+#define CONFIG_BOOTP_BOOTPATH
+
+You should also set the default IP address of your board and the server
+as well as the default file to load when a 'bootp' command is issued.
+
+#define CONFIG_IPADDR           10.0.0.2
+#define CONFIG_SERVERIP         10.0.0.1
+#define CONFIG_BOOTFILE         uImage
+
+
+The 'usb start' command should identify the adapter something like this:
+
+CrOS> usb start
+(Re)start USB...
+USB EHCI 1.00
+scanning bus for devices... 3 USB Device(s) found
+       scanning bus for storage devices... 0 Storage Device(s) found
+       scanning bus for ethernet devices... 1 Ethernet Device(s) found
+CrOS> print ethact
+ethact=asx0
+
+You can see that it found an ethernet device and we can print out the
+device name (asx0 in this case).
+
+Then 'bootp' should use it to obtain an IP address from DHCP, perhaps
+something like this:
+
+CrOS> bootp
+Waiting for Ethernet connection... done.
+BOOTP broadcast 1
+BOOTP broadcast 2
+DHCP client bound to address 172.22.73.81
+Using asx0 device
+TFTP from server 172.22.72.144; our IP address is 172.22.73.81
+Filename '/tftpboot/uImage-sjg-seaboard-261347'.
+Load address: 0x40c000
+Loading: #################################################################
+         #################################################################
+         #################################################################
+         ################################################
+done
+Bytes transferred = 3557464 (364858 hex)
+CrOS>
+
+
+MAC Addresses
+-------------
+
+Most Ethernet dongles have a built-in MAC address which is unique in the
+world. This is important so that devices on the network can be
+distinguised from each other. MAC address conflicts are evil and
+generally result in strange and eratic behaviour.
+
+Some boards have USB Ethernet chips on-board, and these sometimes do not
+have an assigned MAC address. In this case it is up to you to assign
+one which is unique. You should obtain a valid MAC address from a range
+assigned to you before you ship the product.
+
+Built-in Ethernet adapters support setting the MAC address by means of
+an ethaddr environment variable for each interface (ethaddr, eth1addr,
+eth2addr). There is similar support on the USB network side, using the
+names usbethaddr, usbeth1addr, etc. They are kept separate since we
+don't want a USB device taking the MAC address of a built-in device or
+vice versa.
+
+So if your USB Ethernet chip doesn't have a MAC address available then
+you must set usbethaddr to a suitable MAC address. At the time of
+writing this functionality is only supported by the SMSC driver.
+
+
+Server Setup
+------------
+
+For setting up the server side (dhcpd, tftpd, nfsd) you might find this
+Chromium OS page useful:
+
+http://www.chromium.org/network-based-development
+
-- 
1.7.3.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 4/5] Put common autoload code into AutoLoad() function
  2011-04-13  0:46 [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Simon Glass
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet Simon Glass
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 3/5] Add documentation for USB Host Networking Simon Glass
@ 2011-04-13  0:46 ` Simon Glass
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 5/5] Allow tftp server to be different from bootp/dhcp server Simon Glass
  2011-04-13  3:44 ` [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Mike Frysinger
  4 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13  0:46 UTC (permalink / raw)
  To: u-boot

This is a small clean-up patch.

TEST=Build U-Boot, try bootp and check it auto-loads.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 net/bootp.c |   76 +++++++++++++++++++++++++---------------------------------
 1 files changed, 33 insertions(+), 43 deletions(-)

diff --git a/net/bootp.c b/net/bootp.c
index 1a71786..a3d0e63 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -137,6 +137,36 @@ static int truncate_sz (const char *name, int maxlen, int curlen)
 	return (curlen);
 }
 
+/*
+ * Check if autoload is enabled. If so, use either NFS or TFTP to download
+ * the boot file.
+ */
+static void auto_load(void)
+{
+	char *s = getenv("autoload");
+
+	if (s != NULL) {
+		if (*s == 'n') {
+			/*
+			 * Just use BOOTP to configure system;
+			 * Do not use TFTP to load the bootfile.
+			 */
+			NetState = NETLOOP_SUCCESS;
+			return;
+#if defined(CONFIG_CMD_NFS)
+		} else if (strcmp(s, "NFS") == 0) {
+			/*
+			 * Use NFS to load the bootfile.
+			 */
+			NfsStart();
+			return;
+#endif
+		}
+	}
+
+	TftpStart();
+}
+
 #if !defined(CONFIG_CMD_DHCP)
 
 static void BootpVendorFieldProcess (u8 * ext)
@@ -278,6 +308,7 @@ static void BootpVendorProcess (u8 * ext, int size)
 	if (NetBootFileSize)
 		debug("NetBootFileSize: %d\n", NetBootFileSize);
 }
+
 /*
  *	Handle a BOOTP received packet.
  */
@@ -285,7 +316,6 @@ static void
 BootpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len)
 {
 	Bootp_t *bp;
-	char	*s;
 
 	debug("got BOOTP packet (src=%d, dst=%d, len=%d want_len=%zu)\n",
 		src, dest, len, sizeof (Bootp_t));
@@ -312,26 +342,7 @@ BootpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len)
 
 	debug("Got good BOOTP\n");
 
-	if ((s = getenv("autoload")) != NULL) {
-		if (*s == 'n') {
-			/*
-			 * Just use BOOTP to configure system;
-			 * Do not use TFTP to load the bootfile.
-			 */
-			NetState = NETLOOP_SUCCESS;
-			return;
-#if defined(CONFIG_CMD_NFS)
-		} else if (strcmp(s, "NFS") == 0) {
-			/*
-			 * Use NFS to load the bootfile.
-			 */
-			NfsStart();
-			return;
-#endif
-		}
-	}
-
-	TftpStart();
+	auto_load();
 }
 #endif
 
@@ -904,34 +915,13 @@ DhcpHandler(uchar * pkt, unsigned dest, unsigned src, unsigned len)
 		debug("DHCP State: REQUESTING\n");
 
 		if ( DhcpMessageType((u8 *)bp->bp_vend) == DHCP_ACK ) {
-			char *s;
-
 			if (NetReadLong((ulong*)&bp->bp_vend[0]) == htonl(BOOTP_VENDOR_MAGIC))
 				DhcpOptionsProcess((u8 *)&bp->bp_vend[4], bp);
 			BootpCopyNetParams(bp); /* Store net params from reply */
 			dhcp_state = BOUND;
 			printf ("DHCP client bound to address %pI4\n", &NetOurIP);
 
-			/* Obey the 'autoload' setting */
-			if ((s = getenv("autoload")) != NULL) {
-				if (*s == 'n') {
-					/*
-					 * Just use BOOTP to configure system;
-					 * Do not use TFTP to load the bootfile.
-					 */
-					NetState = NETLOOP_SUCCESS;
-					return;
-#if defined(CONFIG_CMD_NFS)
-				} else if (strcmp(s, "NFS") == 0) {
-					/*
-					 * Use NFS to load the bootfile.
-					 */
-					NfsStart();
-					return;
-#endif
-				}
-			}
-			TftpStart();
+			auto_load();
 			return;
 		}
 		break;
-- 
1.7.3.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 5/5] Allow tftp server to be different from bootp/dhcp server
  2011-04-13  0:46 [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Simon Glass
                   ` (2 preceding siblings ...)
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 4/5] Put common autoload code into AutoLoad() function Simon Glass
@ 2011-04-13  0:46 ` Simon Glass
  2011-04-13  3:44 ` [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Mike Frysinger
  4 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13  0:46 UTC (permalink / raw)
  To: u-boot

In many environments the DHCP server (which provides IP addresses) is not
under control of the individual engineer. While it is required in order to
connect to the corporate network, it doesn't provide useful information for
booting. For example, it may be set up for PC imaging and provide a bootfile
and tftp server for pxelinux.

To deal with this, this commit provides for a separate tftpserverip environment
variable. If this is defined, then this IP address is used in preference
to serverip. A new CONFIG_BOOTP_IGNORE_BOOTFILE option is provided to indicate
that any bootfile name returned by the DHCP server is bogus and should be
ignored.

To use this feature, just setenv tftpserverip to the address of your TFTP
server and define CONFIG_BOOTP_IGNORE_BOOTFILE in your board file.

To use a unified DHCP and TFTP server, don't do either of these steps.

TEST=build U-Boot, try bootp with and without tftpserverip set and with and
without CONFIG_BOOTP_IGNORE_BOOTFILE.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 net/bootp.c |    3 ++-
 net/net.c   |   12 ++++++++++--
 net/tftp.c  |    2 +-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/bootp.c b/net/bootp.c
index a3d0e63..1f4523f 100644
--- a/net/bootp.c
+++ b/net/bootp.c
@@ -113,9 +113,10 @@ static void BootpCopyNetParams(Bootp_t *bp)
 		NetCopyIP(&NetServerIP, &bp->bp_siaddr);
 	memcpy (NetServerEther, ((Ethernet_t *)NetRxPacket)->et_src, 6);
 #endif
+#if !defined(CONFIG_BOOTP_IGNORE_BOOTFILE)
 	if (strlen(bp->bp_file) > 0)
 		copy_filename (BootFile, bp->bp_file, sizeof(BootFile));
-
+#endif
 	debug("Bootfile: %s\n", BootFile);
 
 	/* Propagate to environment:
diff --git a/net/net.c b/net/net.c
index a609632..77bd192 100644
--- a/net/net.c
+++ b/net/net.c
@@ -138,6 +138,7 @@ uchar		NetServerEther[6] =	/* Boot server enet address		*/
 			{ 0, 0, 0, 0, 0, 0 };
 IPaddr_t	NetOurIP;		/* Our IP addr (0 = unknown)		*/
 IPaddr_t	NetServerIP;		/* Server IP addr (0 = unknown)		*/
+IPaddr_t	NetTftpServerIP;	/* Tftp Server IP addr (0 = unknown) */
 volatile uchar *NetRxPacket;		/* Current receive packet		*/
 int		NetRxPacketLen;		/* Current rx packet length		*/
 unsigned	NetIPID;		/* IP packet ID				*/
@@ -289,6 +290,7 @@ NetInitLoop(proto_t protocol)
 		NetOurGatewayIP = getenv_IPaddr ("gatewayip");
 		NetOurSubnetMask= getenv_IPaddr ("netmask");
 		NetServerIP = getenv_IPaddr ("serverip");
+		NetTftpServerIP = getenv_IPaddr("tftpserverip");
 		NetOurNativeVLAN = getenv_VLAN("nvlan");
 		NetOurVLAN = getenv_VLAN("vlan");
 #if defined(CONFIG_CMD_DNS)
@@ -1720,8 +1722,14 @@ static int net_check_prereq (proto_t protocol)
 #endif
 	case NETCONS:
 	case TFTP:
-		if (NetServerIP == 0) {
-			puts ("*** ERROR: `serverip' not set\n");
+		/*
+		 * If there is no specific Tftp server defined, just use the
+		 * generic one */
+		if (NetTftpServerIP == 0)
+			NetTftpServerIP = NetServerIP;
+		if (NetTftpServerIP == 0) {
+			puts("*** ERROR: `serverip' and 'tcpserverip'"
+				"not set\n");
 			return (1);
 		}
 #if defined(CONFIG_CMD_PING) || defined(CONFIG_CMD_SNTP)
diff --git a/net/tftp.c b/net/tftp.c
index ed559b7..4f6b1a2 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -547,7 +547,7 @@ TftpStart (void)
 	debug("TFTP blocksize = %i, timeout = %ld ms\n",
 		TftpBlkSizeOption, TftpTimeoutMSecs);
 
-	TftpServerIP = NetServerIP;
+	TftpServerIP = NetTftpServerIP;
 	if (BootFile[0] == '\0') {
 		sprintf(default_filename, "%02lX%02lX%02lX%02lX.img",
 			NetOurIP & 0xFF,
-- 
1.7.3.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13  0:46 [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Simon Glass
                   ` (3 preceding siblings ...)
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 5/5] Allow tftp server to be different from bootp/dhcp server Simon Glass
@ 2011-04-13  3:44 ` Mike Frysinger
  2011-04-13 18:47   ` Simon Glass
  4 siblings, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-13  3:44 UTC (permalink / raw)
  To: u-boot

On Tuesday, April 12, 2011 20:46:08 Simon Glass wrote:
> +static int turbo_mode = 1;

perhaps this should be a #define ?  i cant see anything that changes its value 
in this driver ...

> +static void smsc95xx_halt(struct eth_device *eth)
> +{
> +	debug("** %s()\n", __func__);
> +}

is this right ?  shouldnt you be halting something ?

> +/*
> + * SMSC probing functions
> + */
> +void smsc95xx_eth_before_probe(void)
> +{
> +	curr_eth_dev = 0;
> +}

curr_eth_dev is declared static which means it starts off with a value of 0.  
when does before_probe get called ?  just once ever during the execution of u-
boot ?  i guess this func should get tossed if that's the case.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110412/88b6bd4c/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13  0:46 ` [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet Simon Glass
@ 2011-04-13  3:48   ` Mike Frysinger
  2011-04-13 20:23     ` Albert ARIBAUD
  0 siblings, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-13  3:48 UTC (permalink / raw)
  To: u-boot

On Tuesday, April 12, 2011 20:46:09 Simon Glass wrote:
> --- a/include/net.h
> +++ b/include/net.h
> +/* Tftp Server IP addr (0 = unknown)*/
> +extern IPaddr_t		NetTftpServerIP;

doesnt seem to belong in this patch.  guess it bled in from another ?

> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -254,31 +287,12 @@
>  				eth_current = dev;
>  				puts (" [PRIME]");
>  			}
> -
>  			if (strchr(dev->name, ' '))
> -				puts("\nWarning: eth device name has a space!\n");
> +				puts("\nWarning: eth device name has a "
> +					"space!\n");

please dont change unrelated things.  that newline should be there, and this 
string shouldnt be broken up.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110412/f8062dee/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13  3:44 ` [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Mike Frysinger
@ 2011-04-13 18:47   ` Simon Glass
  2011-04-13 19:45     ` Wolfgang Denk
  2011-04-13 21:08     ` Mike Frysinger
  0 siblings, 2 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13 18:47 UTC (permalink / raw)
  To: u-boot

On Tue, Apr 12, 2011 at 8:44 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday, April 12, 2011 20:46:08 Simon Glass wrote:
>> +static int turbo_mode = 1;
>
> perhaps this should be a #define ? ?i cant see anything that changes its value
> in this driver ...

OK, I have changed this to a compile-time option.

>> +static void smsc95xx_halt(struct eth_device *eth)
>> +{
>> + ? ? debug("** %s()\n", __func__);
>> +}
>
> is this right ? ?shouldnt you be halting something ?

I could do, but halting is not in the driver at present. Do you think
it is important?

>> +/*
>> + * SMSC probing functions
>> + */
>> +void smsc95xx_eth_before_probe(void)
>> +{
>> + ? ? curr_eth_dev = 0;
>> +}
>
> curr_eth_dev is declared static which means it starts off with a value of 0.
> when does before_probe get called ? ?just once ever during the execution of u-
> boot ? ?i guess this func should get tossed if that's the case.

It is called on a 'usb start'. I think we should keep it so people can
call this repeatedly without problems.

Regards,
Simon

> -mike
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13 18:47   ` Simon Glass
@ 2011-04-13 19:45     ` Wolfgang Denk
  2011-04-13 21:13       ` Simon Glass
  2011-04-13 21:08     ` Mike Frysinger
  1 sibling, 1 reply; 27+ messages in thread
From: Wolfgang Denk @ 2011-04-13 19:45 UTC (permalink / raw)
  To: u-boot

Dear Simon Glass,

In message <BANLkTinacFQ=1xA4PTgZKi5ON9AGG7-Jwg@mail.gmail.com> you wrote:
>
> >> +static void smsc95xx_halt(struct eth_device *eth)
> >> +{
> >> +     debug("** %s()\n", __func__);
> >> +}
> >
> > is this right ?  shouldnt you be halting something ?
> 
> I could do, but halting is not in the driver at present. Do you think
> it is important?

Yes, it is important to shut down devices.  Especially with USB
devices.

BTW: can you *please* fix your mail addresses?
It's not "Remy Bohmerl <inux@bohmer.net>"
but "Remy Bohmer <linux@bohmer.net>" instead.  Don't you get tons of
bounce messages?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Every living thing wants to survive.
	-- Spock, "The Ultimate Computer", stardate 4731.3

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13  3:48   ` Mike Frysinger
@ 2011-04-13 20:23     ` Albert ARIBAUD
  2011-04-13 20:28       ` Simon Glass
                         ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Albert ARIBAUD @ 2011-04-13 20:23 UTC (permalink / raw)
  To: u-boot

Le 13/04/2011 05:48, Mike Frysinger a ?crit :

>>   			if (strchr(dev->name, ' '))
>> -				puts("\nWarning: eth device name has a space!\n");
>> +				puts("\nWarning: eth device name has a "
>> +					"space!\n");
>
> please dont change unrelated things.  that newline should be there, and this
> string shouldnt be broken up.

Technically it is not. A C string constant can be made of several 
quote-delimited string literals separated by whitespace -- so this 
change is a no-op, and does not remove any newline btw. I suspect the 
change is to keep checkpatch.pl happy about the line length.

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13 20:23     ` Albert ARIBAUD
@ 2011-04-13 20:28       ` Simon Glass
  2011-04-13 21:05       ` Mike Frysinger
  2011-04-13 23:30       ` Mike Frysinger
  2 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13 20:28 UTC (permalink / raw)
  To: u-boot

On Wed, Apr 13, 2011 at 1:23 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Le 13/04/2011 05:48, Mike Frysinger a ?crit :
>
>>> ? ? ? ? ? ? ? ? ? ? ?if (strchr(dev->name, ' '))
>>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ?puts("\nWarning: eth device name has a space!\n");
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?puts("\nWarning: eth device name has a "
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"space!\n");
>>
>> please dont change unrelated things. ?that newline should be there, and this
>> string shouldnt be broken up.
>
> Technically it is not. A C string constant can be made of several
> quote-delimited string literals separated by whitespace -- so this
> change is a no-op, and does not remove any newline btw. I suspect the
> change is to keep checkpatch.pl happy about the line length.

Yes it was, but on-line reviewers are noisier than checkpatch so have
changed it. Also I was actually reverting a previous change so there
is no need for my change at all. But thank you for your kind
understanding.

I do find it quite confusing adding code which fits with U-Boot's
coding style to a file which doesn't :-)

Regards,
Simon

>
> Amicalement,
> --
> Albert.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13 20:23     ` Albert ARIBAUD
  2011-04-13 20:28       ` Simon Glass
@ 2011-04-13 21:05       ` Mike Frysinger
  2011-04-14  5:01         ` Albert ARIBAUD
  2011-04-13 23:30       ` Mike Frysinger
  2 siblings, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-13 21:05 UTC (permalink / raw)
  To: u-boot

On Wednesday, April 13, 2011 16:23:20 Albert ARIBAUD wrote:
> Le 13/04/2011 05:48, Mike Frysinger a ?crit :
> >>   			if (strchr(dev->name, ' '))
> >> 
> >> -				puts("\nWarning: eth device name has a space!\n");
> >> +				puts("\nWarning: eth device name has a "
> >> +					"space!\n");
> > 
> > please dont change unrelated things.  that newline should be there, and
> > this string shouldnt be broken up.
> 
> Technically it is not. A C string constant can be made of several
> quote-delimited string literals separated by whitespace -- so this
> change is a no-op, and does not remove any newline btw. I suspect the
> change is to keep checkpatch.pl happy about the line length.

you truncated my quote which changes things significantly.  i wasnt talking 
about the string when i said newline.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110413/5baa3e96/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13 18:47   ` Simon Glass
  2011-04-13 19:45     ` Wolfgang Denk
@ 2011-04-13 21:08     ` Mike Frysinger
  2011-04-13 22:31       ` Simon Glass
  1 sibling, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-13 21:08 UTC (permalink / raw)
  To: u-boot

On Wednesday, April 13, 2011 14:47:10 Simon Glass wrote:
> On Tue, Apr 12, 2011 at 8:44 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Tuesday, April 12, 2011 20:46:08 Simon Glass wrote:
> >> +/*
> >> + * SMSC probing functions
> >> + */
> >> +void smsc95xx_eth_before_probe(void)
> >> +{
> >> +     curr_eth_dev = 0;
> >> +}
> > 
> > curr_eth_dev is declared static which means it starts off with a value of
> > 0. when does before_probe get called ?  just once ever during the
> > execution of u- boot ?  i guess this func should get tossed if that's
> > the case.
> 
> It is called on a 'usb start'. I think we should keep it so people can
> call this repeatedly without problems.

as long as the devices created in between are also punted, this should be 
fine.  i was thinking of this case as you dont want:
 - usb start
 - probe some usb net devices (get usbnet0 and usbnet1)
 - usb start
 - previous usbnet0/usbnet1 still exist, and the counter has been reset to 0 
so the new devices start back at usbnet0

if there's something in the bigger picture that takes care of this already, 
then this is probably fine.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110413/072e80c6/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13 19:45     ` Wolfgang Denk
@ 2011-04-13 21:13       ` Simon Glass
  2011-04-13 21:21         ` Simon Glass
  0 siblings, 1 reply; 27+ messages in thread
From: Simon Glass @ 2011-04-13 21:13 UTC (permalink / raw)
  To: u-boot

On Wed, Apr 13, 2011 at 12:45 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <BANLkTinacFQ=1xA4PTgZKi5ON9AGG7-Jwg@mail.gmail.com> you wrote:
>>
>> >> +static void smsc95xx_halt(struct eth_device *eth)
>> >> +{
>> >> + ? ? debug("** %s()\n", __func__);
>> >> +}
>> >
>> > is this right ? ?shouldnt you be halting something ?
>>
>> I could do, but halting is not in the driver at present. Do you think
>> it is important?
>
> Yes, it is important to shut down devices. ?Especially with USB
> devices.

OK I will put something together. I am finding the rebase juggling a
little tricky so there will be a delay while I sort that out so I can
test properly.

> BTW: can you *please* fix your mail addresses?
> It's not "Remy Bohmerl <inux@bohmer.net>"
> but "Remy Bohmer <linux@bohmer.net>" instead. ?Don't you get tons of
> bounce messages?

Have fixed it. No bounces yet, maybe you have a brother? :-)

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13 21:13       ` Simon Glass
@ 2011-04-13 21:21         ` Simon Glass
  2011-04-13 21:43           ` Simon Glass
  2011-04-13 21:50           ` Wolfgang Denk
  0 siblings, 2 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13 21:21 UTC (permalink / raw)
  To: u-boot

On Wed, Apr 13, 2011 at 2:13 PM, Simon Glass <sjg@chromium.org> wrote:
> On Wed, Apr 13, 2011 at 12:45 PM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Simon Glass,
>>
>>> I could do, but halting is not in the driver at present. Do you think
>>> it is important?
>>
>> Yes, it is important to shut down devices. ?Especially with USB
>> devices.
>
> OK I will put something together. I am finding the rebase juggling a
> little tricky so there will be a delay while I sort that out so I can
> test properly.

Actually on second thoughts this actually conflicts which what you are
telling me on the rejected patch.

If the Ethernet adapter shuts down when halt() is called, then
presumably it will lose the link. Then when a tftp  comes along, there
is a delay while it re-establishes the link.

So we have:

bootp   (calls init, send, recv, halt)
tftp     (calls init, send, recv, halt)

If halt takes the link down then there will be a delay, which is what
my rejected patch was trying to work around.

So I think either halt should leave the link up, or we need a way to
not call init/halt all the time.

Thoughts?

Regards,
Simon

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13 21:21         ` Simon Glass
@ 2011-04-13 21:43           ` Simon Glass
  2011-04-13 21:50           ` Wolfgang Denk
  1 sibling, 0 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13 21:43 UTC (permalink / raw)
  To: u-boot

On Wed, Apr 13, 2011 at 2:21 PM, Simon Glass <sjg@chromium.org> wrote:
> On Wed, Apr 13, 2011 at 2:13 PM, Simon Glass <sjg@chromium.org> wrote:
>> On Wed, Apr 13, 2011 at 12:45 PM, Wolfgang Denk <wd@denx.de> wrote:
>>> Dear Simon Glass,
>>>
>>>> I could do, but halting is not in the driver at present. Do you think
>>>> it is important?
>>>
>>> Yes, it is important to shut down devices. ?Especially with USB
>>> devices.
...
> So I think either halt should leave the link up, or we need a way to
> not call init/halt all the time.

And another note....'usb stop' will stop the device. So I feel that
leaving the link up is OK in this case, and I believe this is the
behavior of non-USB ethernet devices. I would prefer to leave halt()
as a no-op.

Regards,
Simon

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13 21:21         ` Simon Glass
  2011-04-13 21:43           ` Simon Glass
@ 2011-04-13 21:50           ` Wolfgang Denk
  1 sibling, 0 replies; 27+ messages in thread
From: Wolfgang Denk @ 2011-04-13 21:50 UTC (permalink / raw)
  To: u-boot

Dear Simon Glass,

In message <BANLkTi=oUMockMG45vV6jo9WpMe+OQJxoA@mail.gmail.com> you wrote:
>
> >> Yes, it is important to shut down devices. =A0Especially with USB
> >> devices.
...
> Actually on second thoughts this actually conflicts which what you are
> telling me on the rejected patch.

Not really.

> If the Ethernet adapter shuts down when halt() is called, then
> presumably it will lose the link. Then when a tftp  comes along, there
> is a delay while it re-establishes the link.
> 
> So we have:
> 
> bootp   (calls init, send, recv, halt)
> tftp     (calls init, send, recv, halt)
> 
> If halt takes the link down then there will be a delay, which is what
> my rejected patch was trying to work around.
> 
> So I think either halt should leave the link up, or we need a way to
> not call init/halt all the time.

The second is probably true.  See the discussion we had about this
when Stefano Babic implemented netconsole over ethernet over USB.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
panic: kernel trap (ignored)

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter
  2011-04-13 21:08     ` Mike Frysinger
@ 2011-04-13 22:31       ` Simon Glass
  0 siblings, 0 replies; 27+ messages in thread
From: Simon Glass @ 2011-04-13 22:31 UTC (permalink / raw)
  To: u-boot

On Wed, Apr 13, 2011 at 2:08 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> as long as the devices created in between are also punted, this should be
> fine. ?i was thinking of this case as you dont want:
> ?- usb start
> ?- probe some usb net devices (get usbnet0 and usbnet1)
> ?- usb start
> ?- previous usbnet0/usbnet1 still exist, and the counter has been reset to 0
> so the new devices start back at usbnet0
>
> if there's something in the bigger picture that takes care of this already,
> then this is probably fine.

Yes if you do 'usb start' then the old devices go away.

Regards,
Simon

> -mike
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13 20:23     ` Albert ARIBAUD
  2011-04-13 20:28       ` Simon Glass
  2011-04-13 21:05       ` Mike Frysinger
@ 2011-04-13 23:30       ` Mike Frysinger
  2011-04-14  5:58         ` Albert ARIBAUD
  2 siblings, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-13 23:30 UTC (permalink / raw)
  To: u-boot

On Wednesday, April 13, 2011 16:23:20 Albert ARIBAUD wrote:
> btw. I suspect the change is to keep checkpatch.pl happy about the line
> length.

also, checkpatch is a tool in the toolbox.  people should not be blindly 
following it, but reviewing its output to see what should be changed and which 
should be ignored.

if checkpatch is complaining about code that you arent changing, then you 
probably shouldnt worry about it.  especially when the only thing you're doing 
is changing style.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110413/364226d6/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13 21:05       ` Mike Frysinger
@ 2011-04-14  5:01         ` Albert ARIBAUD
  0 siblings, 0 replies; 27+ messages in thread
From: Albert ARIBAUD @ 2011-04-14  5:01 UTC (permalink / raw)
  To: u-boot

Le 13/04/2011 23:05, Mike Frysinger a ?crit :
> On Wednesday, April 13, 2011 16:23:20 Albert ARIBAUD wrote:
>> Le 13/04/2011 05:48, Mike Frysinger a ?crit :
>>>>    			if (strchr(dev->name, ' '))
>>>>
>>>> -				puts("\nWarning: eth device name has a space!\n");
>>>> +				puts("\nWarning: eth device name has a "
>>>> +					"space!\n");
>>>
>>> please dont change unrelated things.  that newline should be there, and
>>> this string shouldnt be broken up.
>>
>> Technically it is not. A C string constant can be made of several
>> quote-delimited string literals separated by whitespace -- so this
>> change is a no-op, and does not remove any newline btw. I suspect the
>> change is to keep checkpatch.pl happy about the line length.
>
> you truncated my quote which changes things significantly.  i wasnt talking
> about the string when i said newline.
> -mike

Sorry about that -- I did truncate indeed, to keep your comment and the 
change that was immediately before it, which I thought the comment was 
referring to exclusively.

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-13 23:30       ` Mike Frysinger
@ 2011-04-14  5:58         ` Albert ARIBAUD
  2011-04-14  6:12           ` Mike Frysinger
  0 siblings, 1 reply; 27+ messages in thread
From: Albert ARIBAUD @ 2011-04-14  5:58 UTC (permalink / raw)
  To: u-boot

Le 14/04/2011 01:30, Mike Frysinger a ?crit :
> On Wednesday, April 13, 2011 16:23:20 Albert ARIBAUD wrote:
>> btw. I suspect the change is to keep checkpatch.pl happy about the line
>> length.
>
> also, checkpatch is a tool in the toolbox.  people should not be blindly
> following it, but reviewing its output to see what should be changed and which
> should be ignored.
>
> if checkpatch is complaining about code that you arent changing, then you
> probably shouldnt worry about it.  especially when the only thing you're doing
> is changing style.

I tend to see this "don't worry about some checkpatch.pl messages" 
appraoch as similar to "don't worry about some C compiler warnings". in 
that indeed "you probably shouldn't worry about it", and the key is 
"probably": when it bites you back later on, you realize you "probably" 
should have worried. If you apply a zero-C-warning policy, then a 
zero-checkpatch-warning policy makes sense as well...

... with the exception of Linux-centric warning or a coding style 
warning which would conflict with U-Boot's coding style -- anyone 
interested in introducing 'flavors' or 'style' in checkpatch.pl, with 
oone Linux and one U-Boot flavor/style to begin with?

So ignoring /some specific/ checkpatch.pl diagnostic is ok, but that's 
as long as it is established that the specific diagnostic is purely 
linux-centric" or voluntarily ignored as a coding rule; but then we'd 
need a list of such 'non-warnings' somewhere on the Wiki, I think, along 
with a rationale for ignoring it.

> -mike

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-14  5:58         ` Albert ARIBAUD
@ 2011-04-14  6:12           ` Mike Frysinger
  2011-04-14  6:51             ` Albert ARIBAUD
  0 siblings, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-14  6:12 UTC (permalink / raw)
  To: u-boot

On Thursday, April 14, 2011 01:58:32 Albert ARIBAUD wrote:
> Le 14/04/2011 01:30, Mike Frysinger a ?crit :
> > On Wednesday, April 13, 2011 16:23:20 Albert ARIBAUD wrote:
> >> btw. I suspect the change is to keep checkpatch.pl happy about the line
> >> length.
> > 
> > also, checkpatch is a tool in the toolbox.  people should not be blindly
> > following it, but reviewing its output to see what should be changed and
> > which should be ignored.
> > 
> > if checkpatch is complaining about code that you arent changing, then you
> > probably shouldnt worry about it.  especially when the only thing you're
> > doing is changing style.
> 
> I tend to see this "don't worry about some checkpatch.pl messages"
> appraoch as similar to "don't worry about some C compiler warnings". in
> that indeed "you probably shouldn't worry about it", and the key is
> "probably": when it bites you back later on, you realize you "probably"
> should have worried. If you apply a zero-C-warning policy, then a
> zero-checkpatch-warning policy makes sense as well...

how about when it's plain wrong ?  or it's applying a rule that (most of the 
time) is correct, but not *all* the time ?  or it complains about code that 
your patch isnt touching (as is the case here) ?  or it complains about code 
that is being imported (from linux or other projects) ?

so i stand by my statement that checkpatch is a tool and does *not* get the 
final say.  blindly following a tool is good -- if you're blind.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110414/b76f0c76/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-14  6:12           ` Mike Frysinger
@ 2011-04-14  6:51             ` Albert ARIBAUD
  2011-04-15  7:56               ` Mike Frysinger
  0 siblings, 1 reply; 27+ messages in thread
From: Albert ARIBAUD @ 2011-04-14  6:51 UTC (permalink / raw)
  To: u-boot

Le 14/04/2011 08:12, Mike Frysinger a ?crit :
> On Thursday, April 14, 2011 01:58:32 Albert ARIBAUD wrote:
>> Le 14/04/2011 01:30, Mike Frysinger a ?crit :
>>> On Wednesday, April 13, 2011 16:23:20 Albert ARIBAUD wrote:
>>>> btw. I suspect the change is to keep checkpatch.pl happy about the line
>>>> length.
>>>
>>> also, checkpatch is a tool in the toolbox.  people should not be blindly
>>> following it, but reviewing its output to see what should be changed and
>>> which should be ignored.
>>>
>>> if checkpatch is complaining about code that you arent changing, then you
>>> probably shouldnt worry about it.  especially when the only thing you're
>>> doing is changing style.
>>
>> I tend to see this "don't worry about some checkpatch.pl messages"
>> appraoch as similar to "don't worry about some C compiler warnings". in
>> that indeed "you probably shouldn't worry about it", and the key is
>> "probably": when it bites you back later on, you realize you "probably"
>> should have worried. If you apply a zero-C-warning policy, then a
>> zero-checkpatch-warning policy makes sense as well...
>
> how about when it's plain wrong ?  or it's applying a rule that (most of the
> time) is correct, but not *all* the time ?  or it complains about code that
> your patch isnt touching (as is the case here) ?  or it complains about code
> that is being imported (from linux or other projects) ?

As for "plain wrong" warnings, I think this was addressed in the two 
paragraphs of my answer which followed the one you quote here. :)

As for rule exceptions, nothing prohibits submitters from stating the 
number of checkpatch warnings in their patch and justifying why they did 
not resolve them.

As for code the patch is not touching, again, it could be one of these 
established exceptions -- but again, it has to be a global decision so 
that reviewers don't emit contradicting requests about it.

As for code imported from Linux or any other project, there is an 
exception already, right on the second paragraph of the Wiki page on 
coding style <http://www.denx.de/wiki/U-Boot/CodingStyle>.

> so i stand by my statement that checkpatch is a tool and does *not* get the
> final say.  blindly following a tool is good -- if you're blind.

When it emits warnings, the C toolchain is also just a tool and does 
also not have the final say ; that's why they are warnings and not 
errors, because they might indicate something wrong, or not.

Besides, the coding rules for U-Boot make the tool checkpatch.pl 
mandatory, and I don't think it means "run it then don't care if some 
warnings are there"; I think it means "run it and make sure you 
eliminate all warnings if you can, and if there are some left, either 
justify them when you submit the patch, or ask about them on the list".

As for the 'blind' part, please re-read the two paragraphs right after 
the one you commented here; I believe it makes it clear that I do not 
advocate blindness, but on the contrary, strictness in which warnings we 
would agree to turn a blind eye to, and which we would not.

> -mike

Anyway, I think we've both made our opinions clear; to avoid spending 
too much time here, I guess the best is that Wolfgang, who has the final 
say :), states exactly how he intends submitters and reviewers to handle 
checkpatch diagnostics.

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-14  6:51             ` Albert ARIBAUD
@ 2011-04-15  7:56               ` Mike Frysinger
  2011-04-15  8:13                 ` Albert ARIBAUD
  0 siblings, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-15  7:56 UTC (permalink / raw)
  To: u-boot

On Thursday, April 14, 2011 02:51:42 Albert ARIBAUD wrote:
> Le 14/04/2011 08:12, Mike Frysinger a ?crit :
> > so i stand by my statement that checkpatch is a tool and does *not* get
> > the final say.  blindly following a tool is good -- if you're blind.
> 
> When it emits warnings, the C toolchain is also just a tool and does
> also not have the final say ; that's why they are warnings and not
> errors, because they might indicate something wrong, or not.

sounds like you agree with me.  nothing left to say :p.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110415/2d5b62fe/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-15  7:56               ` Mike Frysinger
@ 2011-04-15  8:13                 ` Albert ARIBAUD
  2011-04-15  8:25                   ` Mike Frysinger
  0 siblings, 1 reply; 27+ messages in thread
From: Albert ARIBAUD @ 2011-04-15  8:13 UTC (permalink / raw)
  To: u-boot

Le 15/04/2011 09:56, Mike Frysinger a ?crit :
> On Thursday, April 14, 2011 02:51:42 Albert ARIBAUD wrote:
>> Le 14/04/2011 08:12, Mike Frysinger a ?crit :
>>> so i stand by my statement that checkpatch is a tool and does *not* get
>>> the final say.  blindly following a tool is good -- if you're blind.
>>
>> When it emits warnings, the C toolchain is also just a tool and does
>> also not have the final say ; that's why they are warnings and not
>> errors, because they might indicate something wrong, or not.
>
> sounds like you agree with me.  nothing left to say :p.
> -mike

I agree about those being tools. What I disagree upon is that just 
because they're tools we should disregard their warnings, or more 
precisely, I advocate setting common rules about which warnings we can 
disregard and which ones we cannot -- after all, the common rules are 
there already for C warnings and they're quite simple :)

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-15  8:13                 ` Albert ARIBAUD
@ 2011-04-15  8:25                   ` Mike Frysinger
  2011-04-15  9:34                     ` Albert ARIBAUD
  0 siblings, 1 reply; 27+ messages in thread
From: Mike Frysinger @ 2011-04-15  8:25 UTC (permalink / raw)
  To: u-boot

On Friday, April 15, 2011 04:13:11 Albert ARIBAUD wrote:
> What I disagree upon is that just because they're tools we should disregard
> their warnings

i never said any such thing
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110415/380264fe/attachment.pgp 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet
  2011-04-15  8:25                   ` Mike Frysinger
@ 2011-04-15  9:34                     ` Albert ARIBAUD
  0 siblings, 0 replies; 27+ messages in thread
From: Albert ARIBAUD @ 2011-04-15  9:34 UTC (permalink / raw)
  To: u-boot

Le 15/04/2011 10:25, Mike Frysinger a ?crit :
> On Friday, April 15, 2011 04:13:11 Albert ARIBAUD wrote:
>> What I disagree upon is that just because they're tools we should disregard
>> their warnings
>
> i never said any such thing
> -mike

My bad. What I disagree upon is that just because they're tools we 
should disregard *some of* their warnings.

More precisely, which checkpatch warnings must/should [not] be 
disregarded should not be a matter of personal choice but a coding style 
requirement.

Amicalement,
-- 
Albert.

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2011-04-15  9:34 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-13  0:46 [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Simon Glass
2011-04-13  0:46 ` [U-Boot] [PATCH v2 2/5] Add Ethernet hardware MAC address framework to usbnet Simon Glass
2011-04-13  3:48   ` Mike Frysinger
2011-04-13 20:23     ` Albert ARIBAUD
2011-04-13 20:28       ` Simon Glass
2011-04-13 21:05       ` Mike Frysinger
2011-04-14  5:01         ` Albert ARIBAUD
2011-04-13 23:30       ` Mike Frysinger
2011-04-14  5:58         ` Albert ARIBAUD
2011-04-14  6:12           ` Mike Frysinger
2011-04-14  6:51             ` Albert ARIBAUD
2011-04-15  7:56               ` Mike Frysinger
2011-04-15  8:13                 ` Albert ARIBAUD
2011-04-15  8:25                   ` Mike Frysinger
2011-04-15  9:34                     ` Albert ARIBAUD
2011-04-13  0:46 ` [U-Boot] [PATCH v2 3/5] Add documentation for USB Host Networking Simon Glass
2011-04-13  0:46 ` [U-Boot] [PATCH v2 4/5] Put common autoload code into AutoLoad() function Simon Glass
2011-04-13  0:46 ` [U-Boot] [PATCH v2 5/5] Allow tftp server to be different from bootp/dhcp server Simon Glass
2011-04-13  3:44 ` [U-Boot] [PATCH v2 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter Mike Frysinger
2011-04-13 18:47   ` Simon Glass
2011-04-13 19:45     ` Wolfgang Denk
2011-04-13 21:13       ` Simon Glass
2011-04-13 21:21         ` Simon Glass
2011-04-13 21:43           ` Simon Glass
2011-04-13 21:50           ` Wolfgang Denk
2011-04-13 21:08     ` Mike Frysinger
2011-04-13 22:31       ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.