[U-Boot] [PATCH v2 0/7] usb: eth: introduce Moschip MCS7830 driver

this series - does a little cleanup: BIT() and BITMASK() macros in common.h, no #ifdef for routine declarations in header files - adds a new USB ethernet driver for adapters that are based on the MCS7730/7830/7832 chips - enables the driver for those boards which previously had support for "all other" USB ethernet adapters - updates the README.usb documentation file to list all available drivers for USB ethernet adapters
development and tests were done on a taskit stamp9g20 with Delock and Logilink adapters, running TFTP transfers of a file as large as RAM is
there are several checkpatch warnings - about CamelCase for NetReceive() and USB related structure members, which cannot get fixed as the names are in the established API - about a multiple assignment for the "found" flags in the USB endpoint search, which I consider acceptable
version v2 addresses all received feedback: - adjust the Cc: list (network custodian, BIT() macro committers, board maintainers) - move the BIT() macro to the common.h header file - reduce the number of timeouts, only "USB call" and "network link status" timeouts are left - use errno.h codes instead of the unhelpful "-1" magic number - factor out common PHY access code, fix the retry logic - more straight forward init() callback and link status check, silent operation according to the U-Boot philosophy - remove text fold markers - kernel nano-doc style comments for data structures, global variables, and routines - remove #ifdef from function prototypes in header files - whitespace fixes (in the USB dongle list comments) - switch the USB dongle list from sentinel terminated to "dense, useful data only" and iterate by means of ARRAY_SIZE() - remove an ugly and non-obvious "unused" silencer, instead use the __maybe_unused decoration near the implementation
Gerhard Sittig (7): include: move the BIT() macro into the common.h header file usb: eth: don't ifdef routine declarations in usb_ether.h usb: eth: introduce support for Moschip USB ethernet config: alpha-sort USB ethernet items for Asix and SMSC config: enable Moschip USB ethernet support for several boards config: enable USB ethernet for taskit stamp9g20 usb: doc: update README.usb to list all USB ethernet options
arch/arm/cpu/arm1176/tnetv107x/clock.c | 2 - arch/arm/cpu/arm926ejs/davinci/cpu.c | 2 - arch/arm/include/asm/arch-am33xx/cpu.h | 3 +- arch/arm/include/asm/arch-ixp/ixp425.h | 2 +- arch/arm/include/asm/arch-omap5/cpu.h | 4 +- arch/arm/include/asm/arch-tegra20/dc.h | 4 +- doc/README.usb | 13 +- drivers/mtd/nand/jz4740_nand.c | 1 - drivers/net/cpsw.c | 1 - drivers/net/npe/include/IxOsalOsIxp400.h | 2 +- drivers/spi/andes_spi.h | 4 +- drivers/spi/davinci_spi.h | 4 +- drivers/usb/eth/Makefile | 1 + drivers/usb/eth/mcs7830.c | 850 ++++++++++++++++++++++++++++++ drivers/usb/eth/usb_ether.c | 7 + include/common.h | 3 + include/configs/harmony.h | 3 +- include/configs/m53evk.h | 1 + include/configs/mx53loco.h | 1 + include/configs/nitrogen6x.h | 1 + include/configs/omap3_beagle.h | 3 +- include/configs/stamp9g20.h | 5 +- include/usb_ether.h | 14 +- 23 files changed, 903 insertions(+), 28 deletions(-) create mode 100644 drivers/usb/eth/mcs7830.c

several compile units and local header files re-invented the BIT() macro, move BIT() and BITMASK() declarations into the common.h header file and adjust call sites
Cc: Cyril Chemparathy cyril@ti.com Cc: David Brownell dbrownell@users.sourceforge.net Cc: Chandan Nath chandan.nath@ti.com Cc: Wolfgang Denk wd@denx.de Cc: Mugunthan V N mugunthanvnm@ti.com Cc: Wei Ni wni@nvidia.com Cc: Xiangfu Liu xiangfu@openmobilefree.net Cc: Macpaul Lin macpaul@andestech.com Cc: Sekhar Nori nsekhar@ti.com Signed-off-by: Gerhard Sittig gsi@denx.de --- arch/arm/cpu/arm1176/tnetv107x/clock.c | 2 -- arch/arm/cpu/arm926ejs/davinci/cpu.c | 2 -- arch/arm/include/asm/arch-am33xx/cpu.h | 3 ++- arch/arm/include/asm/arch-ixp/ixp425.h | 2 +- arch/arm/include/asm/arch-omap5/cpu.h | 4 ++-- arch/arm/include/asm/arch-tegra20/dc.h | 4 ++-- drivers/mtd/nand/jz4740_nand.c | 1 - drivers/net/cpsw.c | 1 - drivers/net/npe/include/IxOsalOsIxp400.h | 2 +- drivers/spi/andes_spi.h | 4 ++-- drivers/spi/davinci_spi.h | 4 ++-- include/common.h | 3 +++ 12 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/arch/arm/cpu/arm1176/tnetv107x/clock.c b/arch/arm/cpu/arm1176/tnetv107x/clock.c index 3708b6f59f49..a30b666b4d76 100644 --- a/arch/arm/cpu/arm1176/tnetv107x/clock.c +++ b/arch/arm/cpu/arm1176/tnetv107x/clock.c @@ -13,8 +13,6 @@ #define CLOCK_BASE TNETV107X_CLOCK_CONTROL_BASE #define PSC_BASE TNETV107X_PSC_BASE
-#define BIT(x) (1 << (x)) - #define MAX_PREDIV 64 #define MAX_POSTDIV 8 #define MAX_MULT 512 diff --git a/arch/arm/cpu/arm926ejs/davinci/cpu.c b/arch/arm/cpu/arm926ejs/davinci/cpu.c index ff6114775728..74c3d5d936c2 100644 --- a/arch/arm/cpu/arm926ejs/davinci/cpu.c +++ b/arch/arm/cpu/arm926ejs/davinci/cpu.c @@ -28,8 +28,6 @@ DECLARE_GLOBAL_DATA_PTR; #define PLLC_PLLDIV8 0x170 #define PLLC_PLLDIV9 0x174
-#define BIT(x) (1 << (x)) - /* SOC-specific pll info */ #ifdef CONFIG_SOC_DM355 #define ARM_PLLDIV PLLC_PLLDIV1 diff --git a/arch/arm/include/asm/arch-am33xx/cpu.h b/arch/arm/include/asm/arch-am33xx/cpu.h index 9febfa2719a9..4df504a12939 100644 --- a/arch/arm/include/asm/arch-am33xx/cpu.h +++ b/arch/arm/include/asm/arch-am33xx/cpu.h @@ -11,13 +11,14 @@ #ifndef _AM33XX_CPU_H #define _AM33XX_CPU_H
+#include <common.h> + #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__)) #include <asm/types.h> #endif /* !(__KERNEL_STRICT_NAMES || __ASSEMBLY__) */
#include <asm/arch/hardware.h>
-#define BIT(x) (1 << x) #define CL_BIT(x) (0 << x)
/* Timer register bits */ diff --git a/arch/arm/include/asm/arch-ixp/ixp425.h b/arch/arm/include/asm/arch-ixp/ixp425.h index c2e9c820495a..3bceb445b0ff 100644 --- a/arch/arm/include/asm/arch-ixp/ixp425.h +++ b/arch/arm/include/asm/arch-ixp/ixp425.h @@ -14,7 +14,7 @@ #ifndef _ASM_ARM_IXP425_H_ #define _ASM_ARM_IXP425_H_
-#define BIT(x) (1<<(x)) +#include <common.h>
/* FIXME: Only this does work for u-boot... find out why... [RS] */ #define UBOOT_REG_FIX 1 diff --git a/arch/arm/include/asm/arch-omap5/cpu.h b/arch/arm/include/asm/arch-omap5/cpu.h index fb5a568b698b..7cd245bca915 100644 --- a/arch/arm/include/asm/arch-omap5/cpu.h +++ b/arch/arm/include/asm/arch-omap5/cpu.h @@ -10,6 +10,8 @@ #ifndef _CPU_H #define _CPU_H
+#include <common.h> + #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__)) #include <asm/types.h> #endif /* !(__KERNEL_STRICT_NAMES || __ASSEMBLY__) */ @@ -99,8 +101,6 @@ struct watchdog { #endif /* __ASSEMBLY__ */ #endif /* __KERNEL_STRICT_NAMES */
-#define BIT(x) (1 << (x)) - #define WD_UNLOCK1 0xAAAA #define WD_UNLOCK2 0x5555
diff --git a/arch/arm/include/asm/arch-tegra20/dc.h b/arch/arm/include/asm/arch-tegra20/dc.h index 20790b6c0e90..f1a67e8b63d9 100644 --- a/arch/arm/include/asm/arch-tegra20/dc.h +++ b/arch/arm/include/asm/arch-tegra20/dc.h @@ -8,6 +8,8 @@ #ifndef __ASM_ARCH_TEGRA_DC_H #define __ASM_ARCH_TEGRA_DC_H
+#include <common.h> + /* Register definitions for the Tegra display controller */
/* CMD register 0x000 ~ 0x43 */ @@ -351,8 +353,6 @@ struct dc_ctlr { struct dc_winbuf_reg winbuf; /* WINBUF A/B/C 0x800 ~ 0x80a */ };
-#define BIT(pos) (1U << pos) - /* DC_CMD_DISPLAY_COMMAND 0x032 */ #define CTRL_MODE_SHIFT 5 #define CTRL_MODE_MASK (0x3 << CTRL_MODE_SHIFT) diff --git a/drivers/mtd/nand/jz4740_nand.c b/drivers/mtd/nand/jz4740_nand.c index 7a62cc33613c..abcedc210211 100644 --- a/drivers/mtd/nand/jz4740_nand.c +++ b/drivers/mtd/nand/jz4740_nand.c @@ -16,7 +16,6 @@ #define JZ_NAND_CMD_ADDR (JZ_NAND_DATA_ADDR + 0x8000) #define JZ_NAND_ADDR_ADDR (JZ_NAND_DATA_ADDR + 0x10000)
-#define BIT(x) (1 << (x)) #define JZ_NAND_ECC_CTRL_ENCODING BIT(3) #define JZ_NAND_ECC_CTRL_RS BIT(2) #define JZ_NAND_ECC_CTRL_RESET BIT(1) diff --git a/drivers/net/cpsw.c b/drivers/net/cpsw.c index 50167aab63a8..94b9b9e71086 100644 --- a/drivers/net/cpsw.c +++ b/drivers/net/cpsw.c @@ -26,7 +26,6 @@ #include <phy.h> #include <asm/arch/cpu.h>
-#define BITMASK(bits) (BIT(bits) - 1) #define PHY_REG_MASK 0x1f #define PHY_ID_MASK 0x1f #define NUM_DESCS (PKTBUFSRX * 2) diff --git a/drivers/net/npe/include/IxOsalOsIxp400.h b/drivers/net/npe/include/IxOsalOsIxp400.h index 104c733e3b84..dec8ebb00224 100644 --- a/drivers/net/npe/include/IxOsalOsIxp400.h +++ b/drivers/net/npe/include/IxOsalOsIxp400.h @@ -23,7 +23,7 @@ #ifndef IxOsalOsIxp400_H #define IxOsalOsIxp400_H
-#define BIT(x) (1<<(x)) +#include <common.h>
#define IXP425_EthA_BASE 0xc8009000 #define IXP425_EthB_BASE 0xc800a000 diff --git a/drivers/spi/andes_spi.h b/drivers/spi/andes_spi.h index b7d294599a17..26c6250d9a90 100644 --- a/drivers/spi/andes_spi.h +++ b/drivers/spi/andes_spi.h @@ -10,6 +10,8 @@ #ifndef __ANDES_SPI_H #define __ANDES_SPI_H
+#include <common.h> + struct andes_spi_regs { unsigned int apb; /* 0x00 - APB SPI interface setting */ unsigned int pio; /* 0x04 - PIO reg */ @@ -23,8 +25,6 @@ struct andes_spi_regs { unsigned int ver; /* 0x3c - SPI version reg */ };
-#define BIT(x) (1 << (x)) - /* 0x00 - APB SPI interface setting register */ #define ANDES_SPI_APB_BAUD(x) (((x) & 0xff) < 0) #define ANDES_SPI_APB_CSHT(x) (((x) & 0xf) < 16) diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h index 33f69b5ca98a..1901faefc0e7 100644 --- a/drivers/spi/davinci_spi.h +++ b/drivers/spi/davinci_spi.h @@ -9,6 +9,8 @@ #ifndef _DAVINCI_SPI_H_ #define _DAVINCI_SPI_H_
+#include <common.h> + struct davinci_spi_regs { dv_reg gcr0; /* 0x00 */ dv_reg gcr1; /* 0x04 */ @@ -36,8 +38,6 @@ struct davinci_spi_regs { dv_reg intvec1; /* 0x64 */ };
-#define BIT(x) (1 << (x)) - /* SPIGCR0 */ #define SPIGCR0_SPIENA_MASK 0x1 #define SPIGCR0_SPIRST_MASK 0x0 diff --git a/include/common.h b/include/common.h index 221b7768c5be..49885f3c01bf 100644 --- a/include/common.h +++ b/include/common.h @@ -967,6 +967,9 @@ static inline phys_addr_t map_to_sysmem(const void *ptr) #define ALIGN(x,a) __ALIGN_MASK((x),(typeof(x))(a)-1) #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
+#define BIT(n) (1U << (n)) +#define BITMASK(bits) (BIT(bits) - 1) + /* * ARCH_DMA_MINALIGN is defined in asm/cache.h for each architecture. It * is used to align DMA buffers.

Dear Gerhard Sittig,
In message 1392665727-5734-2-git-send-email-gsi@denx.de you wrote:
several compile units and local header files re-invented the BIT() macro, move BIT() and BITMASK() declarations into the common.h header file and adjust call sites
...
diff --git a/include/common.h b/include/common.h index 221b7768c5be..49885f3c01bf 100644 --- a/include/common.h +++ b/include/common.h @@ -967,6 +967,9 @@ static inline phys_addr_t map_to_sysmem(const void *ptr) #define ALIGN(x,a) __ALIGN_MASK((x),(typeof(x))(a)-1) #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
+#define BIT(n) (1U << (n)) +#define BITMASK(bits) (BIT(bits) - 1)
I'm sorry, but these macros are ugly and make the code harder to read. Also, they are inherently non-portable. I guess you are not aware that "bit 0" is the MSB on Power architecture, not the LSB as you expect in your definition.
I strongly reommend NOT to use any such macros. Adding these to common.h could be considered as an invitation to use them, which is the opposite of what I want.
So sorry, but this is a clear NAK.
Best regards,
Wolfgang Denk

On Mon, Feb 17, 2014 at 22:01 +0100, Wolfgang Denk wrote:
Dear Gerhard Sittig,
In message 1392665727-5734-2-git-send-email-gsi@denx.de you wrote:
several compile units and local header files re-invented the BIT() macro, move BIT() and BITMASK() declarations into the common.h header file and adjust call sites
...
diff --git a/include/common.h b/include/common.h index 221b7768c5be..49885f3c01bf 100644 --- a/include/common.h +++ b/include/common.h @@ -967,6 +967,9 @@ static inline phys_addr_t map_to_sysmem(const void *ptr) #define ALIGN(x,a) __ALIGN_MASK((x),(typeof(x))(a)-1) #define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask))
+#define BIT(n) (1U << (n)) +#define BITMASK(bits) (BIT(bits) - 1)
I'm sorry, but these macros are ugly and make the code harder to read. Also, they are inherently non-portable. I guess you are not aware that "bit 0" is the MSB on Power architecture, not the LSB as you expect in your definition.
I strongly reommend NOT to use any such macros. Adding these to common.h could be considered as an invitation to use them, which is the opposite of what I want.
So sorry, but this is a clear NAK.
That's fine with me. So this patch can get dropped and nothing changes for the existing code base.
The MCS7830 driver will then need a respin (after I took in some more feedback, of course). There was the question of whether to use the BIT(5) macro or re-invented (1 << 5) or 0x20 numbers. The feedback now allows a clear answer. :)
virtually yours Gerhard Sittig

while compilation of implemented routines and references from calling sites may be optional, declarations in header files should not be
unconditionally declare the Asix and SMSC related public USB ethernet driver routines in the usb_ether.h header file
Cc: Simon Glass sjg@chromium.org Signed-off-by: Gerhard Sittig gsi@denx.de --- include/usb_ether.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/usb_ether.h b/include/usb_ether.h index 678c9dff2524..011ead7a364e 100644 --- a/include/usb_ether.h +++ b/include/usb_ether.h @@ -40,23 +40,19 @@ struct ueth_data { };
/* - * Function definitions for each USB ethernet driver go here, bracketed by - * #ifdef CONFIG_USB_ETHER_xxx...#endif + * Function definitions for each USB ethernet driver go here + * (declaration is unconditional, compilation is conditional) */ -#ifdef CONFIG_USB_ETHER_ASIX void asix_eth_before_probe(void); int asix_eth_probe(struct usb_device *dev, unsigned int ifnum, struct ueth_data *ss); int asix_eth_get_info(struct usb_device *dev, struct ueth_data *ss, struct eth_device *eth); -#endif
-#ifdef CONFIG_USB_ETHER_SMSC95XX void smsc95xx_eth_before_probe(void); int smsc95xx_eth_probe(struct usb_device *dev, unsigned int ifnum, struct ueth_data *ss); int smsc95xx_eth_get_info(struct usb_device *dev, struct ueth_data *ss, struct eth_device *eth); -#endif
#endif /* __USB_ETHER_H__ */

On 17 February 2014 12:35, Gerhard Sittig gsi@denx.de wrote:
while compilation of implemented routines and references from calling sites may be optional, declarations in header files should not be
unconditionally declare the Asix and SMSC related public USB ethernet driver routines in the usb_ether.h header file
Cc: Simon Glass sjg@chromium.org Signed-off-by: Gerhard Sittig gsi@denx.de
Acked-by: Simon Glass sjg@chromium.org

introduce an 'mcs7830' driver for Moschip based USB ethernet adapters, which was implemented based on the U-Boot Asix driver with additional information gathered from the Moschip Linux driver
Cc: Joe Hershberger joe.hershberger@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marex@denx.de Signed-off-by: Gerhard Sittig gsi@denx.de --- drivers/usb/eth/Makefile | 1 + drivers/usb/eth/mcs7830.c | 850 +++++++++++++++++++++++++++++++++++++++++++ drivers/usb/eth/usb_ether.c | 7 + include/usb_ether.h | 6 + 4 files changed, 864 insertions(+) create mode 100644 drivers/usb/eth/mcs7830.c
diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile index 03f54749f720..94551c4c0c9a 100644 --- a/drivers/usb/eth/Makefile +++ b/drivers/usb/eth/Makefile @@ -8,4 +8,5 @@ obj-$(CONFIG_USB_HOST_ETHER) += usb_ether.o ifdef CONFIG_USB_ETHER_ASIX obj-y += asix.o endif +obj-$(CONFIG_USB_ETHER_MCS7830) += mcs7830.o obj-$(CONFIG_USB_ETHER_SMSC95XX) += smsc95xx.o diff --git a/drivers/usb/eth/mcs7830.c b/drivers/usb/eth/mcs7830.c new file mode 100644 index 000000000000..991e2ee8f424 --- /dev/null +++ b/drivers/usb/eth/mcs7830.c @@ -0,0 +1,850 @@ +/* + * Copyright (c) 2013 Gerhard Sittig gsi@denx.de + * based on the U-Boot Asix driver as well as information + * from the Linux Moschip driver + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +/* + * MOSCHIP MCS7830 based (7730/7830/7832) USB 2.0 Ethernet Devices + */ + +#include <common.h> +#include <errno.h> +#include <linux/mii.h> +#include <malloc.h> +#include <usb.h> + +#include "usb_ether.h" + +#define MCS7830_BASE_NAME "mcs" + +#define USBCALL_TIMEOUT 1000 +#define LINKSTATUS_TIMEOUT 5000 /* link status, connect timeout */ +#define LINKSTATUS_TIMEOUT_RES 50 /* link status, resolution in msec */ + +#define MCS7830_RX_URB_SIZE 2048 + +/* command opcodes */ +#define MCS7830_WR_BREQ 0x0d +#define MCS7830_RD_BREQ 0x0e + +/* register layout, numerical offset specs for USB API calls */ +struct mcs7830_regs { + uint8_t multicast_hashes[8]; + uint8_t packet_gap[2]; + uint8_t phy_data[2]; + uint8_t phy_command[2]; + uint8_t configuration; + uint8_t ether_address[6]; + uint8_t frame_drop_count; + uint8_t pause_threshold; +}; +#define REG_MULTICAST_HASH offsetof(struct mcs7830_regs, multicast_hashes) +#define REG_PHY_DATA offsetof(struct mcs7830_regs, phy_data) +#define REG_PHY_CMD offsetof(struct mcs7830_regs, phy_command) +#define REG_CONFIG offsetof(struct mcs7830_regs, configuration) +#define REG_ETHER_ADDR offsetof(struct mcs7830_regs, ether_address) +#define REG_FRAME_DROP_COUNTER offsetof(struct mcs7830_regs, frame_drop_count) +#define REG_PAUSE_THRESHOLD offsetof(struct mcs7830_regs, pause_threshold) + +/* bit masks and default values for the above registers */ +#define PHY_CMD1_READ BIT(6) +#define PHY_CMD1_WRITE BIT(5) +#define PHY_CMD1_PHYADDR BIT(0) + +#define PHY_CMD2_PEND BIT(7) +#define PHY_CMD2_READY BIT(6) + +#define CONF_CFG BIT(7) +#define CONF_SPEED100 BIT(6) +#define CONF_FDX_ENABLE BIT(5) +#define CONF_RXENABLE BIT(4) +#define CONF_TXENABLE BIT(3) +#define CONF_SLEEPMODE BIT(2) +#define CONF_ALLMULTICAST BIT(1) +#define CONF_PROMISCUOUS BIT(0) + +#define PAUSE_THRESHOLD_DEFAULT 0 + +/* bit masks for the status byte which follows received ethernet frames */ +#define STAT_RX_FRAME_CORRECT BIT(5) +#define STAT_RX_LARGE_FRAME BIT(4) +#define STAT_RX_CRC_ERROR BIT(3) +#define STAT_RX_ALIGNMENT_ERROR BIT(2) +#define STAT_RX_LENGTH_ERROR BIT(1) +#define STAT_RX_SHORT_FRAME BIT(0) + +/* + * struct mcs7830_private - private driver data for an individual adapter + * @config: shadow for the network adapter's configuration register + * @mchash: shadow for the network adapter's multicast hash registers + */ +struct mcs7830_private { + uint8_t config; + uint8_t mchash[8]; +}; + +/* + * mcs7830_read_reg() - read a register of the network adapter + * @dev: network device to read from + * @idx: index of the register to start reading from + * @size: number of bytes to read + * @data: buffer to read into + * Return: zero upon success, negative upon error + */ +static int mcs7830_read_reg(struct ueth_data *dev, uint8_t idx, + uint16_t size, void *data) +{ + int len; + ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, size); + + debug("%s() idx=0x%04X sz=%d\n", __func__, idx, size); + + len = usb_control_msg(dev->pusb_dev, + usb_rcvctrlpipe(dev->pusb_dev, 0), + MCS7830_RD_BREQ, + USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + 0, idx, buf, size, + USBCALL_TIMEOUT); + if (len != size) { + debug("%s() len=%d != sz=%d\n", __func__, len, size); + return -EIO; + } + memcpy(data, buf, size); + return 0; +} + +/* + * mcs7830_write_reg() - write a register of the network adapter + * @dev: network device to write to + * @idx: index of the register to start writing to + * @size: number of bytes to write + * @data: buffer holding the data to write + * Return: zero upon success, negative upon error + */ +static int mcs7830_write_reg(struct ueth_data *dev, uint8_t idx, + uint16_t size, void *data) +{ + int len; + ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, size); + + debug("%s() idx=0x%04X sz=%d\n", __func__, idx, size); + + memcpy(buf, data, size); + len = usb_control_msg(dev->pusb_dev, + usb_sndctrlpipe(dev->pusb_dev, 0), + MCS7830_WR_BREQ, + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + 0, idx, buf, size, + USBCALL_TIMEOUT); + if (len != size) { + debug("%s() len=%d != sz=%d\n", __func__, len, size); + return -EIO; + } + return 0; +} + +/* + * mcs7830_phy_emit_wait() - emit PHY read/write access, wait for its execution + * @dev: network device to talk to + * @rwflag: PHY_CMD1_READ or PHY_CMD1_WRITE opcode + * @index: number of the PHY register to read or write + * Return: zero upon success, negative upon error + */ +static int mcs7830_phy_emit_wait(struct ueth_data *dev, + uint8_t rwflag, uint8_t index) +{ + int rc; + int retry; + uint8_t cmd[2]; + + /* send the PHY read/write request */ + cmd[0] = rwflag | PHY_CMD1_PHYADDR; + cmd[1] = PHY_CMD2_PEND | (index & 0x1f); + rc = mcs7830_write_reg(dev, REG_PHY_CMD, sizeof(cmd), cmd); + if (rc < 0) + return rc; + + /* wait for the response to become available (usually < 1ms) */ + retry = 10; + do { + rc = mcs7830_read_reg(dev, REG_PHY_CMD, sizeof(cmd), cmd); + if (rc < 0) + return rc; + if (cmd[1] & PHY_CMD2_READY) + return 0; + if (!retry--) + return -ETIMEDOUT; + mdelay(1); + } while (1); + /* UNREACH */ +} + +/* + * mcs7830_read_phy() - read a PHY register of the network adapter + * @dev: network device to read from + * @index: index of the PHY register to read from + * Return: non-negative 16bit register content, negative upon error + */ +static int mcs7830_read_phy(struct ueth_data *dev, uint8_t index) +{ + int rc; + uint16_t val; + + /* send the PHY read request */ + /* issue the PHY read request and wait for its execution */ + rc = mcs7830_phy_emit_wait(dev, PHY_CMD1_READ, index); + if (rc < 0) + return rc; + + /* fetch the PHY data which was read */ + rc = mcs7830_read_reg(dev, REG_PHY_DATA, sizeof(val), &val); + if (rc < 0) + return rc; + rc = le16_to_cpu(val); + debug("%s(%s, %d) => 0x%04X\n", __func__, dev->eth_dev.name, index, rc); + return rc; +} + +/* + * mcs7830_write_phy() - write a PHY register of the network adapter + * @dev: network device to write to + * @index: index of the PHY register to write to + * @val: value to write to the PHY register + * Return: zero upon success, negative upon error + */ +static int mcs7830_write_phy(struct ueth_data *dev, uint8_t index, uint16_t val) +{ + int rc; + + debug("%s(%s, %d, 0x%04X)\n", __func__, dev->eth_dev.name, index, val); + + /* setup the PHY data which is to get written */ + val = cpu_to_le16(val); + rc = mcs7830_write_reg(dev, REG_PHY_DATA, sizeof(val), &val); + if (rc < 0) + return rc; + + /* issue the PHY write request and wait for its execution */ + rc = mcs7830_phy_emit_wait(dev, PHY_CMD1_WRITE, index); + if (rc < 0) + return rc; + + return 0; +} + +/* + * mcs7830_read_config() - read the network adapter's config register + * @eth: network device to read from + * Return: non-negative register content upon success, negative upon error + */ +static int __maybe_unused mcs7830_read_config(struct eth_device *eth) +{ + struct ueth_data *dev; + int rc; + uint8_t cfg; + + debug("%s()\n", __func__); + dev = eth->priv; + + rc = mcs7830_read_reg(dev, REG_CONFIG, sizeof(cfg), &cfg); + if (rc < 0) { + debug("reading config from adapter failed\n"); + return rc; + } + + debug("%s() reg=0x%02X [ %s %s %s %s %s %s %s %s ]\n", + __func__, cfg, + (cfg & CONF_CFG) ? "cfg" : "-", + (cfg & CONF_SPEED100) ? "100" : "-", + (cfg & CONF_FDX_ENABLE) ? "fdx" : "-", + (cfg & CONF_RXENABLE) ? "rx" : "-", + (cfg & CONF_TXENABLE) ? "tx" : "-", + (cfg & CONF_SLEEPMODE) ? "sleep" : "-", + (cfg & CONF_ALLMULTICAST) ? "multi" : "-", + (cfg & CONF_PROMISCUOUS) ? "promisc" : "-"); + return cfg; +} + +/* + * mcs7830_write_config() - write to the network adapter's config register + * @eth: network device to write to + * Return: zero upon success, negative upon error + * + * the data which gets written is taken from the shadow config register + * within the device driver's private data + */ +static int mcs7830_write_config(struct ueth_data *dev) +{ + struct mcs7830_private *priv; + int rc; + + debug("%s()\n", __func__); + priv = dev->dev_priv; + + rc = mcs7830_write_reg(dev, REG_CONFIG, + sizeof(priv->config), &priv->config); + if (rc < 0) { + debug("writing config to adapter failed\n"); + return rc; + } + + return 0; +} + +/* + * mcs7830_write_mchash() - write the network adapter's multicast filter + * @eth: network device to write to + * Return: zero upon success, negative upon error + * + * the data which gets written is taken from the shadow multicast hashes + * within the device driver's private data + */ +static int mcs7830_write_mchash(struct ueth_data *dev) +{ + struct mcs7830_private *priv; + int rc; + + debug("%s()\n", __func__); + priv = dev->dev_priv; + + rc = mcs7830_write_reg(dev, REG_MULTICAST_HASH, + sizeof(priv->mchash), &priv->mchash); + if (rc < 0) { + debug("writing multicast hash to adapter failed\n"); + return rc; + } + + return 0; +} + +/* + * mcs7830_set_autoneg() - setup and trigger ethernet link autonegotiation + * @eth: network device to run link negotiation on + * Return: zero upon success, negative upon error + * + * the routine advertises available media and starts autonegotiation + */ +static int mcs7830_set_autoneg(struct ueth_data *dev) +{ + int adv, flg; + int rc; + + debug("%s()\n", __func__); + + /* + * algorithm taken from the Linux driver, which took it from + * "the original mcs7830 version 1.4 driver": + * + * enable all media, reset BMCR, enable auto neg, restart + * auto neg while keeping the enable auto neg flag set + */ + + adv = ADVERTISE_PAUSE_CAP | ADVERTISE_ALL | ADVERTISE_CSMA; + rc = mcs7830_write_phy(dev, MII_ADVERTISE, adv); + + flg = 0; + if (!rc) + rc = mcs7830_write_phy(dev, MII_BMCR, flg); + + flg |= BMCR_ANENABLE; + if (!rc) + rc = mcs7830_write_phy(dev, MII_BMCR, flg); + + flg |= BMCR_ANRESTART; + if (!rc) + rc = mcs7830_write_phy(dev, MII_BMCR, flg); + + return rc; +} + +/* + * mcs7830_get_rev() - identify a network adapter's chip revision + * @eth: network device to identify + * Return: non-negative number, reflecting the revision number + * + * currently, only "rev C and higher" and "below rev C" are needed, so + * the return value is #1 for "below rev C", and #2 for "rev C and above" + */ +static int mcs7830_get_rev(struct ueth_data *dev) +{ + uint8_t buf[2]; + int rc; + int rev; + + /* register 22 is readable in rev C and higher */ + rc = mcs7830_read_reg(dev, REG_FRAME_DROP_COUNTER, sizeof(buf), buf); + if (rc < 0) + rev = 1; + else + rev = 2; + debug("%s() rc=%d, rev=%d\n", __func__, rc, rev); + return rev; +} + +/* + * mcs7830_apply_fixup() - identify an adapter and potentially apply fixups + * @eth: network device to identify and apply fixups to + * Return: zero upon success (no errors emitted from here) + * + * this routine identifies the network adapter's chip revision, and applies + * fixups for known issues + */ +static int mcs7830_apply_fixup(struct ueth_data *dev) +{ + int rev; + int i; + uint8_t thr; + + rev = mcs7830_get_rev(dev); + debug("%s() rev=%d\n", __func__, rev); + + /* + * rev C requires setting the pause threshold (the Linux driver + * is inconsistent, the implementation does it for "rev C + * exactly", the introductory comment says "rev C and above") + */ + if (rev == 2) { + debug("%s: applying rev C fixup\n", dev->eth_dev.name); + thr = PAUSE_THRESHOLD_DEFAULT; + for (i = 0; i < 2; i++) { + (void)mcs7830_write_reg(dev, REG_PAUSE_THRESHOLD, + sizeof(thr), &thr); + mdelay(1); + } + } + + return 0; +} + +/* + * mcs7830_basic_reset() - bring the network adapter into a known first state + * @eth: network device to act upon + * Return: zero upon success, negative upon error + * + * this routine initializes the network adapter such that subsequent invocations + * of the interface callbacks can exchange ethernet frames; link negotiation is + * triggered from here already and continues in background + */ +static int mcs7830_basic_reset(struct ueth_data *dev) +{ + struct mcs7830_private *priv; + int rc; + + debug("%s()\n", __func__); + priv = dev->dev_priv; + + /* + * comment from the respective Linux driver, which + * unconditionally sets the ALLMULTICAST flag as well: + * should not be needed, but does not work otherwise + */ + priv->config = CONF_TXENABLE; + priv->config |= CONF_ALLMULTICAST; + + rc = mcs7830_set_autoneg(dev); + if (rc < 0) { + error("setting autoneg failed\n"); + return rc; + } + + rc = mcs7830_write_mchash(dev); + if (rc < 0) { + error("failed to set multicast hash\n"); + return rc; + } + + rc = mcs7830_write_config(dev); + if (rc < 0) { + error("failed to set configuration\n"); + return rc; + } + + rc = mcs7830_apply_fixup(dev); + if (rc < 0) { + error("fixup application failed\n"); + return rc; + } + + return 0; +} + +/* + * mcs7830_read_mac() - read an ethernet adapter's MAC address + * @eth: network device to read from + * Return: zero upon success, negative upon error + * + * this routine fetches the MAC address stored within the ethernet adapter, + * and stores it in the ethernet interface's data structure + */ +static int mcs7830_read_mac(struct eth_device *eth) +{ + struct ueth_data *dev; + int rc; + uint8_t buf[ETH_ALEN]; + + debug("%s()\n", __func__); + dev = eth->priv; + + rc = mcs7830_read_reg(dev, REG_ETHER_ADDR, ETH_ALEN, buf); + if (rc < 0) { + debug("reading MAC from adapter failed\n"); + return rc; + } + + memcpy(ð->enetaddr[0], buf, ETH_ALEN); + return 0; +} + +/* + * mcs7830_write_mac() - write an ethernet adapter's MAC address + * @eth: network device to write to + * Return: zero upon success, negative upon error + * + * this routine takes the MAC address from the ethernet interface's data + * structure, and writes it into the ethernet adapter such that subsequent + * exchange of ethernet frames uses this address + */ +static int mcs7830_write_mac(struct eth_device *eth) +{ + struct ueth_data *dev; + int rc; + + debug("%s()\n", __func__); + dev = eth->priv; + + if (sizeof(eth->enetaddr) != ETH_ALEN) + return -EINVAL; + rc = mcs7830_write_reg(dev, REG_ETHER_ADDR, ETH_ALEN, eth->enetaddr); + if (rc < 0) { + debug("writing MAC to adapter failed\n"); + return rc; + } + return 0; +} + +/* + * mcs7830_init() - network interface's init callback + * @eth: network device to initialize + * @bd: board information + * Return: zero upon success, negative upon error + * + * after initial setup during probe() and get_info(), this init() callback + * ensures that the link is up and subsequent send() and recv() calls can + * exchange ethernet frames + */ +static int mcs7830_init(struct eth_device *eth, bd_t *bd) +{ + struct ueth_data *dev; + int timeout; + int have_link; + + debug("%s()\n", __func__); + dev = eth->priv; + + timeout = 0; + do { + have_link = mcs7830_read_phy(dev, MII_BMSR) & BMSR_LSTATUS; + if (have_link) + break; + udelay(LINKSTATUS_TIMEOUT_RES * 1000); + timeout += LINKSTATUS_TIMEOUT_RES; + } while (timeout < LINKSTATUS_TIMEOUT); + if (!have_link) { + debug("ethernet link is down\n"); + return -ETIMEDOUT; + } + return 0; +} + +/* + * mcs7830_send() - network interface's send callback + * @eth: network device to send the frame from + * @packet: ethernet frame content + * @length: ethernet frame length + * Return: zero upon success, negative upon error + * + * this routine send an ethernet frame out of the network interface + */ +static int mcs7830_send(struct eth_device *eth, void *packet, int length) +{ + struct ueth_data *dev; + int rc; + int gotlen; + /* there is a status byte after the ethernet frame */ + ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, PKTSIZE + sizeof(uint8_t)); + + dev = eth->priv; + + memcpy(buf, packet, length); + rc = usb_bulk_msg(dev->pusb_dev, + usb_sndbulkpipe(dev->pusb_dev, dev->ep_out), + &buf[0], length, &gotlen, + USBCALL_TIMEOUT); + debug("%s() TX want len %d, got len %d, rc %d\n", + __func__, length, gotlen, rc); + return rc; +} + +/* + * mcs7830_recv() - network interface's recv callback + * @eth: network device to receive frames from + * Return: zero upon success, negative upon error + * + * this routine checks for available ethernet frames that the network + * interface might have received, and notifies the network stack + */ +static int mcs7830_recv(struct eth_device *eth) +{ + struct ueth_data *dev; + ALLOC_CACHE_ALIGN_BUFFER(uint8_t, buf, MCS7830_RX_URB_SIZE); + int rc, wantlen, gotlen; + uint8_t sts; + + debug("%s()\n", __func__); + dev = eth->priv; + + /* fetch input data from the adapter */ + wantlen = MCS7830_RX_URB_SIZE; + rc = usb_bulk_msg(dev->pusb_dev, + usb_rcvbulkpipe(dev->pusb_dev, dev->ep_in), + &buf[0], wantlen, &gotlen, + USBCALL_TIMEOUT); + debug("%s() RX want len %d, got len %d, rc %d\n", + __func__, wantlen, gotlen, rc); + if (rc != 0) { + error("RX: failed to receive\n"); + return rc; + } + if (gotlen > wantlen) { + error("RX: got too many bytes (%d)\n", gotlen); + return -EIO; + } + + /* + * the bulk message that we received from USB contains exactly + * one ethernet frame and a trailing status byte + */ + if (gotlen < sizeof(sts)) + return -EIO; + gotlen -= sizeof(sts); + sts = buf[gotlen]; + + if (sts == STAT_RX_FRAME_CORRECT) { + debug("%s() got a frame, len=%d\n", __func__, gotlen); + NetReceive(buf, gotlen); + return 0; + } + + debug("RX: frame error (sts 0x%02X, %s %s %s %s %s)\n", + sts, + (sts & STAT_RX_LARGE_FRAME) ? "large" : "-", + (sts & STAT_RX_LENGTH_ERROR) ? "length" : "-", + (sts & STAT_RX_SHORT_FRAME) ? "short" : "-", + (sts & STAT_RX_CRC_ERROR) ? "crc" : "-", + (sts & STAT_RX_ALIGNMENT_ERROR) ? "align" : "-"); + return -EIO; +} + +/* + * mcs7830_halt() - network interface's halt callback + * @eth: network device to cease operation of + * Return: none + * + * this routine is supposed to undo the effect of previous initialization and + * ethernet frames exchange; in this implementation it's a NOP + */ +static void mcs7830_halt(struct eth_device *eth) +{ + debug("%s()\n", __func__); +} + +/* + * mcs7830_iface_idx - index of detected network interfaces + * + * this counter keeps track of identified supported interfaces, + * to assign unique names as more interfaces are found + */ +static int mcs7830_iface_idx; + +/* + * mcs7830_eth_before_probe() - network driver's before_probe callback + * Return: none + * + * this routine initializes driver's internal data in preparation of + * subsequent probe callbacks + */ +void mcs7830_eth_before_probe(void) +{ + mcs7830_iface_idx = 0; +} + +/* + * struct mcs7830_dongle - description of a supported Moschip ethernet dongle + * @vendor: 16bit USB vendor identification + * @product: 16bit USB product identification + * + * this structure describes a supported USB ethernet dongle by means of the + * vendor and product codes found during USB enumeration; no flags are held + * here since all supported dongles have identical behaviour, and required + * fixups get determined at runtime, such that no manual configuration is + * needed + */ +struct mcs7830_dongle { + uint16_t vendor; + uint16_t product; +}; + +/* + * mcs7830_dongles - the list of supported Moschip based USB ethernet dongles + */ +static const struct mcs7830_dongle const mcs7830_dongles[] = { + { 0x9710, 0x7832, }, /* Moschip 7832 */ + { 0x9710, 0x7830, }, /* Moschip 7830 */ + { 0x9710, 0x7730, }, /* Moschip 7730 */ + { 0x0df6, 0x0021, }, /* Sitecom LN 30 */ +}; + +/* + * mcs7830_eth_probe() - network driver's probe callback + * @dev: detected USB device to check + * @ifnum: detected USB interface to check + * @ss: USB ethernet data structure to fill in upon match + * Return: #1 upon match, #0 upon mismatch or error + * + * this routine checks whether the found USB device is supported by + * this ethernet driver, and upon match fills in the USB ethernet + * data structure which later is passed to the get_info callback + */ +int mcs7830_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; + struct mcs7830_private *priv; + int ep_in_found, ep_out_found, ep_intr_found; + + debug("%s()\n", __func__); + + /* iterate the list of supported dongles */ + iface = &dev->config.if_desc[ifnum]; + iface_desc = &iface->desc; + for (i = 0; i < ARRAY_SIZE(mcs7830_dongles); i++) { + if (dev->descriptor.idVendor == mcs7830_dongles[i].vendor && + dev->descriptor.idProduct == mcs7830_dongles[i].product) + break; + } + if (i == ARRAY_SIZE(mcs7830_dongles)) + return 0; + debug("detected USB ethernet device: %04X:%04X\n", + dev->descriptor.idVendor, dev->descriptor.idProduct); + + /* fill in driver private data */ + priv = calloc(1, sizeof(*priv)); + if (!priv) + return 0; + + /* fill in the ueth_data structure, attach private data */ + memset(ss, 0, sizeof(*ss)); + ss->ifnum = ifnum; + ss->pusb_dev = dev; + ss->subclass = iface_desc->bInterfaceSubClass; + ss->protocol = iface_desc->bInterfaceProtocol; + ss->dev_priv = priv; + + /* + * a minimum of three endpoints is expected: in (bulk), + * out (bulk), and interrupt; ignore all others + */ + ep_in_found = ep_out_found = ep_intr_found = 0; + for (i = 0; i < iface_desc->bNumEndpoints; i++) { + uint8_t eptype, epaddr; + bool is_input; + + eptype = iface->ep_desc[i].bmAttributes; + eptype &= USB_ENDPOINT_XFERTYPE_MASK; + + epaddr = iface->ep_desc[i].bEndpointAddress; + is_input = epaddr & USB_DIR_IN; + epaddr &= USB_ENDPOINT_NUMBER_MASK; + + if (eptype == USB_ENDPOINT_XFER_BULK) { + if (is_input && !ep_in_found) { + ss->ep_in = epaddr; + ep_in_found++; + } + if (!is_input && !ep_out_found) { + ss->ep_out = epaddr; + ep_out_found++; + } + } + + if (eptype == USB_ENDPOINT_XFER_INT) { + if (is_input && !ep_intr_found) { + ss->ep_int = epaddr; + ss->irqinterval = iface->ep_desc[i].bInterval; + ep_intr_found++; + } + } + } + debug("endpoints: in %d, out %d, intr %d\n", + ss->ep_in, ss->ep_out, ss->ep_int); + + /* apply basic sanity checks */ + if (usb_set_interface(dev, iface_desc->bInterfaceNumber, 0) || + !ss->ep_in || !ss->ep_out || !ss->ep_int) { + debug("device probe incomplete\n"); + return 0; + } + + dev->privptr = ss; + return 1; +} + +/* + * mcs7830_eth_get_info() - network driver's get_info callback + * @dev: detected USB device + * @ss: USB ethernet data structure filled in at probe() + * @eth: ethernet interface data structure to fill in + * Return: #1 upon success, #0 upon error + * + * this routine registers the mandatory init(), send(), recv(), and + * halt() callbacks with the ethernet interface, can register the + * optional write_hwaddr() callback with the ethernet interface, + * and initiates configuration of the interface such that subsequent + * calls to those callbacks results in network communication + */ +int mcs7830_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; + } + + snprintf(eth->name, sizeof(eth->name), "%s%d", + MCS7830_BASE_NAME, mcs7830_iface_idx++); + eth->init = mcs7830_init; + eth->send = mcs7830_send; + eth->recv = mcs7830_recv; + eth->halt = mcs7830_halt; + eth->write_hwaddr = mcs7830_write_mac; + eth->priv = ss; + + if (mcs7830_basic_reset(ss)) + return 0; + +#ifdef DEBUG + (void)mcs7830_read_config(eth); +#endif + + if (mcs7830_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 2c4126be3601..1dda54c2f116 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_MCS7830 + { + .before_probe = mcs7830_eth_before_probe, + .probe = mcs7830_eth_probe, + .get_info = mcs7830_eth_get_info, + }, +#endif #ifdef CONFIG_USB_ETHER_SMSC95XX { .before_probe = smsc95xx_eth_before_probe, diff --git a/include/usb_ether.h b/include/usb_ether.h index 011ead7a364e..35700a21b59f 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 mcs7830_eth_before_probe(void); +int mcs7830_eth_probe(struct usb_device *dev, unsigned int ifnum, + struct ueth_data *ss); +int mcs7830_eth_get_info(struct usb_device *dev, struct ueth_data *ss, + struct eth_device *eth); + void smsc95xx_eth_before_probe(void); int smsc95xx_eth_probe(struct usb_device *dev, unsigned int ifnum, struct ueth_data *ss);

On Monday, February 17, 2014 at 08:35:23 PM, Gerhard Sittig wrote:
[...]
+int mcs7830_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;
- }
- snprintf(eth->name, sizeof(eth->name), "%s%d",
MCS7830_BASE_NAME, mcs7830_iface_idx++);
- eth->init = mcs7830_init;
- eth->send = mcs7830_send;
- eth->recv = mcs7830_recv;
- eth->halt = mcs7830_halt;
- eth->write_hwaddr = mcs7830_write_mac;
- eth->priv = ss;
- if (mcs7830_basic_reset(ss))
return 0;
+#ifdef DEBUG
- (void)mcs7830_read_config(eth);
So this is debug-only function? You might want to put the entire function into #ifdef DEBUG and then have an #else , where you define the function as an empty one. The GCC shall handle the rest then as well, but you won't have this ugly ifdef in a function. [...]
Best regards, Marek Vasut

On Mon, Feb 17, 2014 at 21:57 +0100, Marek Vasut wrote:
On Monday, February 17, 2014 at 08:35:23 PM, Gerhard Sittig wrote:
[...]
+int mcs7830_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;
- }
- snprintf(eth->name, sizeof(eth->name), "%s%d",
MCS7830_BASE_NAME, mcs7830_iface_idx++);
- eth->init = mcs7830_init;
- eth->send = mcs7830_send;
- eth->recv = mcs7830_recv;
- eth->halt = mcs7830_halt;
- eth->write_hwaddr = mcs7830_write_mac;
- eth->priv = ss;
- if (mcs7830_basic_reset(ss))
return 0;
+#ifdef DEBUG
- (void)mcs7830_read_config(eth);
So this is debug-only function? You might want to put the entire function into #ifdef DEBUG and then have an #else , where you define the function as an empty one. The GCC shall handle the rest then as well, but you won't have this ugly ifdef in a function.
I thought about this for a while.
Usually you'd expect separate control and status registers in hardware. Where you write to control, and read back from status. Here those two aspects appear to have been mixed into one "config" register, and only in hindsight the reading became unused. It's not so much an intent, but more of a byproduct.
During development (before the driver became operational), I could not tell whether I had to read-modify-write that config register. Following the Linux driver's approach, currently only fixed values get written to the adapter and nothing gets read back.
Later the shadow in the driver's private data was introduced, such that "updates" neither need to read back what was written before. And since neither multicast nor promiscuous mode may apply to bootloader operation, even those updates may never need not occur.
In the meantime I'd even tend to support the removal of the config register read routine. Adding code "just in case" is a programmer's sin that may not be acceptable in U-Boot, since the cost outweights the benefit.
The current implementation (v3, with "maybe unused" decoration) might be acceptable. But should feedback suggest that v4 is needed, I will remove that routine as well.
virtually yours Gerhard Sittig

On Sunday, February 23, 2014 at 09:16:20 PM, Gerhard Sittig wrote:
On Mon, Feb 17, 2014 at 21:57 +0100, Marek Vasut wrote:
On Monday, February 17, 2014 at 08:35:23 PM, Gerhard Sittig wrote:
[...]
+int mcs7830_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;
- }
- snprintf(eth->name, sizeof(eth->name), "%s%d",
MCS7830_BASE_NAME, mcs7830_iface_idx++);
- eth->init = mcs7830_init;
- eth->send = mcs7830_send;
- eth->recv = mcs7830_recv;
- eth->halt = mcs7830_halt;
- eth->write_hwaddr = mcs7830_write_mac;
- eth->priv = ss;
- if (mcs7830_basic_reset(ss))
return 0;
+#ifdef DEBUG
- (void)mcs7830_read_config(eth);
So this is debug-only function? You might want to put the entire function into #ifdef DEBUG and then have an #else , where you define the function as an empty one. The GCC shall handle the rest then as well, but you won't have this ugly ifdef in a function.
I thought about this for a while.
Usually you'd expect separate control and status registers in hardware. Where you write to control, and read back from status. Here those two aspects appear to have been mixed into one "config" register, and only in hindsight the reading became unused. It's not so much an intent, but more of a byproduct.
During development (before the driver became operational), I could not tell whether I had to read-modify-write that config register. Following the Linux driver's approach, currently only fixed values get written to the adapter and nothing gets read back.
Later the shadow in the driver's private data was introduced, such that "updates" neither need to read back what was written before. And since neither multicast nor promiscuous mode may apply to bootloader operation, even those updates may never need not occur.
In the meantime I'd even tend to support the removal of the config register read routine. Adding code "just in case" is a programmer's sin that may not be acceptable in U-Boot, since the cost outweights the benefit.
The current implementation (v3, with "maybe unused" decoration) might be acceptable. But should feedback suggest that v4 is needed, I will remove that routine as well.
If you feel like removing the routine, then please remove it .
Best regards, Marek Vasut

Dear Gerhard,
In message 1392665727-5734-4-git-send-email-gsi@denx.de you wrote:
introduce an 'mcs7830' driver for Moschip based USB ethernet adapters, which was implemented based on the U-Boot Asix driver with additional information gathered from the Moschip Linux driver
...
+/* bit masks and default values for the above registers */ +#define PHY_CMD1_READ BIT(6) +#define PHY_CMD1_WRITE BIT(5) +#define PHY_CMD1_PHYADDR BIT(0)
+#define PHY_CMD2_PEND BIT(7) +#define PHY_CMD2_READY BIT(6)
...
As mentioned in patch # 1, I object against the use of these obfuscating BIT() macros. Please do not use these; use plain readable code, that leaves no ambiguities to the reader.
Best regards,
Wolfgang Denk

On Monday, February 17, 2014 at 10:11:17 PM, Wolfgang Denk wrote:
Dear Gerhard,
In message 1392665727-5734-4-git-send-email-gsi@denx.de you wrote:
introduce an 'mcs7830' driver for Moschip based USB ethernet adapters, which was implemented based on the U-Boot Asix driver with additional information gathered from the Moschip Linux driver
...
+/* bit masks and default values for the above registers */ +#define PHY_CMD1_READ BIT(6) +#define PHY_CMD1_WRITE BIT(5) +#define PHY_CMD1_PHYADDR BIT(0)
+#define PHY_CMD2_PEND BIT(7) +#define PHY_CMD2_READY BIT(6)
...
As mentioned in patch # 1, I object against the use of these obfuscating BIT() macros. Please do not use these; use plain readable code, that leaves no ambiguities to the reader.
Just to chime in real quick, Linux uses these 'BIT()' macros, but I personally have no hard feelings about them either way.
Best regards, Marek Vasut

Dear Marek,
In message 201402172222.38911.marex@denx.de you wrote:
+#define PHY_CMD1_READ BIT(6) +#define PHY_CMD1_WRITE BIT(5) +#define PHY_CMD1_PHYADDR BIT(0)
+#define PHY_CMD2_PEND BIT(7) +#define PHY_CMD2_READY BIT(6)
...
As mentioned in patch # 1, I object against the use of these obfuscating BIT() macros. Please do not use these; use plain readable code, that leaves no ambiguities to the reader.
Just to chime in real quick, Linux uses these 'BIT()' macros, but I personally have no hard feelings about them either way.
Yes, certain developers have been using this style before. This does not make it any better. Fact is, that I have no way to tell what the code means. BIT(0) can be expected to have any of the following meanings: 0x01, 0x80, 0x8000, 0x80000000, ... So I always have to look up the macro definition first, before I can unerstand it. And then I have to compute in my head what the number actually means.
Compare: BIT(6) or 0x40 - what is easier to write, to read, and to understand?
You dump a register - either with the BDI, or with some printf(). You get 0x27051956. Is BIT(17) set? Is bit 0x00020000 set? Which of these questions is easier to answer (even when you are sure that this is on a system where bit no. 0 is the LSB)?
It's all about writing portable and maintainable code.
Best regards,
Wolfgang Denk

Hello Wolfgang,
Dear Marek,
In message 201402172222.38911.marex@denx.de you wrote:
+#define PHY_CMD1_READ BIT(6) +#define PHY_CMD1_WRITE BIT(5) +#define PHY_CMD1_PHYADDR BIT(0)
+#define PHY_CMD2_PEND BIT(7) +#define PHY_CMD2_READY BIT(6)
...
As mentioned in patch # 1, I object against the use of these obfuscating BIT() macros. Please do not use these; use plain readable code, that leaves no ambiguities to the reader.
Just to chime in real quick, Linux uses these 'BIT()' macros, but I personally have no hard feelings about them either way.
Yes, certain developers have been using this style before. This does not make it any better. Fact is, that I have no way to tell what the code means. BIT(0) can be expected to have any of the following meanings: 0x01, 0x80, 0x8000, 0x80000000, ... So I always have to look up the macro definition first, before I can unerstand it. And then I have to compute in my head what the number actually means.
Compare: BIT(6) or 0x40 - what is easier to write, to read, and to understand?
(1 << 6) is easier for me to read honestly, because then I can quickly crosscheck it with the datasheet.
You dump a register - either with the BDI, or with some printf(). You get 0x27051956. Is BIT(17) set? Is bit 0x00020000 set? Which of these questions is easier to answer (even when you are sure that this is on a system where bit no. 0 is the LSB)?
You have a point with the endianness here.
Best regards, Marek Vasut

adjust the harmony and omap3_beagle board configs to make their CONFIG_USB_ETHER_* items appear in alphabetical order
Cc: Tom Warren twarren@nvidia.com Cc: Tom Rini trini@ti.com Signed-off-by: Gerhard Sittig gsi@denx.de --- include/configs/harmony.h | 2 +- include/configs/omap3_beagle.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/configs/harmony.h b/include/configs/harmony.h index d733be9cd5b6..fa66c665ec8c 100644 --- a/include/configs/harmony.h +++ b/include/configs/harmony.h @@ -61,8 +61,8 @@
/* USB networking support */ #define CONFIG_USB_HOST_ETHER -#define CONFIG_USB_ETHER_SMSC95XX #define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_SMSC95XX
/* General networking support */ #define CONFIG_CMD_NET diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h index c58bc91a50c5..e01a6d9547f9 100644 --- a/include/configs/omap3_beagle.h +++ b/include/configs/omap3_beagle.h @@ -120,8 +120,8 @@
#define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3 #define CONFIG_USB_HOST_ETHER -#define CONFIG_USB_ETHER_SMSC95XX #define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_SMSC95XX
/* GPIO banks */ #define CONFIG_OMAP3_GPIO_5 /* GPIO128..159 is in GPIO bank 5 */

enable support for the Moschip USB ethernet adapter for those boards which previously had support for "all other" USB ethernet adapters (that's Asix _and_ SMSC) enabled -- which applies to harmony, m53evk, mx53loco, nitrogen6x, omap3_beagle
Cc: Tom Warren twarren@nvidia.com Cc: Marek Vasut marek.vasut@gmail.com Cc: Jason Liu r64343@freescale.com Cc: Eric Nelson eric.nelson@boundarydevices.com Cc: Tom Rini trini@ti.com Signed-off-by: Gerhard Sittig gsi@denx.de --- include/configs/harmony.h | 1 + include/configs/m53evk.h | 1 + include/configs/mx53loco.h | 1 + include/configs/nitrogen6x.h | 1 + include/configs/omap3_beagle.h | 1 + 5 files changed, 5 insertions(+)
diff --git a/include/configs/harmony.h b/include/configs/harmony.h index fa66c665ec8c..34b43faeb079 100644 --- a/include/configs/harmony.h +++ b/include/configs/harmony.h @@ -62,6 +62,7 @@ /* USB networking support */ #define CONFIG_USB_HOST_ETHER #define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_MCS7830 #define CONFIG_USB_ETHER_SMSC95XX
/* General networking support */ diff --git a/include/configs/m53evk.h b/include/configs/m53evk.h index a344af457392..a57ac166ba90 100644 --- a/include/configs/m53evk.h +++ b/include/configs/m53evk.h @@ -183,6 +183,7 @@ #define CONFIG_USB_STORAGE #define CONFIG_USB_HOST_ETHER #define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_MCS7830 #define CONFIG_USB_ETHER_SMSC95XX #define CONFIG_MXC_USB_PORT 1 #define CONFIG_MXC_USB_PORTSC (PORT_PTS_UTMI | PORT_PTS_PTW) diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h index ae43ea3c1f28..df1f377fdd6d 100644 --- a/include/configs/mx53loco.h +++ b/include/configs/mx53loco.h @@ -65,6 +65,7 @@ #define CONFIG_USB_STORAGE #define CONFIG_USB_HOST_ETHER #define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_MCS7830 #define CONFIG_USB_ETHER_SMSC95XX #define CONFIG_MXC_USB_PORT 1 #define CONFIG_MXC_USB_PORTSC (PORT_PTS_UTMI | PORT_PTS_PTW) diff --git a/include/configs/nitrogen6x.h b/include/configs/nitrogen6x.h index e44ec88f71b3..b412cc5cd9ca 100644 --- a/include/configs/nitrogen6x.h +++ b/include/configs/nitrogen6x.h @@ -115,6 +115,7 @@ #define CONFIG_USB_STORAGE #define CONFIG_USB_HOST_ETHER #define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_MCS7830 #define CONFIG_USB_ETHER_SMSC95XX #define CONFIG_USB_MAX_CONTROLLER_COUNT 2 #define CONFIG_EHCI_HCD_INIT_AFTER_RESET /* For OTG port */ diff --git a/include/configs/omap3_beagle.h b/include/configs/omap3_beagle.h index e01a6d9547f9..73eea304d19f 100644 --- a/include/configs/omap3_beagle.h +++ b/include/configs/omap3_beagle.h @@ -121,6 +121,7 @@ #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3 #define CONFIG_USB_HOST_ETHER #define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_MCS7830 #define CONFIG_USB_ETHER_SMSC95XX
/* GPIO banks */

enabling CONFIG_MACB makes other locations in the stamp config file enable network related commands (actually prevents disabling them)
enable USB ethernet support by activating generic support as well as Asix and Moschip ethernet adapters
Cc: Markus Hubig mhubig@imko.de Signed-off-by: Gerhard Sittig gsi@denx.de --- include/configs/stamp9g20.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/configs/stamp9g20.h b/include/configs/stamp9g20.h index 51339b1496e6..01085dc5c114 100644 --- a/include/configs/stamp9g20.h +++ b/include/configs/stamp9g20.h @@ -140,7 +140,10 @@ * can enable it here if your baseboard features ethernet. */
-/* #define CONFIG_MACB */ +#define CONFIG_MACB +#define CONFIG_USB_HOST_ETHER +#define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_MCS7830
#ifdef CONFIG_MACB # define CONFIG_RMII /* use reduced MII inteface */

- extend the discussion of USB network related config options such that all available adapter drivers are listed, and that the 'usb' command for the interactive prompt and scripting becomes available - suggest to *not* put individual IP configuration parameters into the exectuable, but instead to put them into external environment or fetch them from network
Cc: Simon Glass sjg@chromium.org Signed-off-by: Gerhard Sittig gsi@denx.de --- doc/README.usb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/doc/README.usb b/doc/README.usb index 65fb2886d958..bc768a385450 100644 --- a/doc/README.usb +++ b/doc/README.usb @@ -127,8 +127,14 @@ 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_CMD_USB /* the 'usb' interactive command */ #define CONFIG_USB_HOST_ETHER /* Enable USB Ethernet adapters */ -#define CONFIG_USB_ETHER_ASIX /* Asix, or whatever driver(s) you want */ + +and one or more of the following for individual adapter hardware: + +#define CONFIG_USB_ETHER_ASIX +#define CONFIG_USB_ETHER_MCS7830 +#define CONFIG_USB_ETHER_SMSC95XX
As with built-in networking, you will also want to enable some network commands, for example: @@ -148,7 +154,10 @@ settings should start you off:
You can 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. -All of these can be obtained from the bootp server if not set. +However note that encoding these individual network settings into a +common exectuable is discouraged, as it leads to potential conflicts, +and all the parameters can either get stored in the board's external +environment, or get obtained from the bootp server if not set.
#define CONFIG_IPADDR 10.0.0.2 (replace with your value) #define CONFIG_SERVERIP 10.0.0.1 (replace with your value)
participants (4)
-
Gerhard Sittig
-
Marek Vasut
-
Simon Glass
-
Wolfgang Denk