[U-Boot] [PATCH v3 2/2] usb: eth: add Realtek RTL8152B/RTL8153 driver

[Marek Vasut <marex@denx.de>]

On Tuesday, December 01, 2015 at 12:24:41 PM, Ted Chen wrote:
> This patch adds driver support for the Realtek RTL8152B/RTL8153 USB
> network adapters.

Hi!

looks pretty good, just a few final nitpicks below.

> Signed-off-by: Ted Chen <tedchen@realtek.com>
> [swarren, fixed a few compiler warnings]
> [swarren, with permission, converted license header to SPDX]
> [swarren, removed printf() spew during probe()]

This changelog should be removed, really. It doesn't have to be in the commit 
message, right ?

> Signed-off-by: Stephen Warren <swarren@nvidia.com>

[...]

> +#include <common.h>
> +#include <errno.h>
> +#include <malloc.h>
> +#include <usb.h>
> +#include <usb/lin_gadget_compat.h>
> +#include <linux/mii.h>
> +#include <linux/bitops.h>
> +#include "usb_ether.h"
> +#include "r8152.h"
> +
> +/* local vars */
> +static int curr_eth_dev; /* index for name of next device detected */

This $curr_eth_dev doesn't seem very DM friendly. Is this really needed?

> +struct r8152_dongle {
> +	unsigned short vendor;
> +	unsigned short product;
> +};
> +
> +static const struct r8152_dongle const r8152_dongles[] = {
> +	/* Realtek */
> +	{ 0x0bda, 0x8050 },
> +	{ 0x0bda, 0x8152 },
> +	{ 0x0bda, 0x8153 },
> +
> +	/* Samsung */
> +	{ 0x04e8, 0xa101 },
> +
> +	/* Lenovo */
> +	{ 0x17ef, 0x304f },
> +	{ 0x17ef, 0x3052 },
> +	{ 0x17ef, 0x3054 },
> +	{ 0x17ef, 0x3057 },
> +	{ 0x17ef, 0x7205 },
> +	{ 0x17ef, 0x720a },
> +	{ 0x17ef, 0x720b },
> +	{ 0x17ef, 0x720c },
> +
> +	/* TP-LINK */
> +	{ 0x2357, 0x0601 },
> +
> +	/* Nvidia */
> +	{ 0x0955, 0x09ff },
> +
> +	{ 0x0000, 0x0000 }	/* END - Do not remove */
> +};
> +
> +static
> +int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void
> *data) +{
> +	int ret;
> +
> +	ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
> +			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> +			      value, index, data, size, 500);
> +
> +	return ret;
> +}
> +
> +static
> +int set_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void
> *data) +{
> +	int ret;
> +
> +	ret = usb_control_msg(tp->udev, usb_sndctrlpipe(tp->udev, 0),
> +			      RTL8152_REQ_SET_REGS, RTL8152_REQT_WRITE,
> +			      value, index, data, size, 500);
> +
> +	return ret;
> +}
> +
> +int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
> +		     void *data, u16 type)
> +{
> +	u16 burst_size = 64;
> +	int ret = 0;

int txsize;

> +	/* both size and index must be 4 bytes align */
> +	if ((size & 3) || !size || (index & 3) || !data)
> +		return -EINVAL;
> +
> +	if (index + size > 0xffff)
> +		return -EINVAL;
> +
> +	while (size) {

txsize = min(size, burst_size);

ret = get_registers(tp, index, type, txsize, data);
if (ret < 0)
 break;
index += txsize;
data += txsize;
size =- txsize;

And you can drop this whole conditional statement. Right ?

> +		if (size > burst_size) {
> +			ret = get_registers(tp, index, type, burst_size, data);
> +			if (ret < 0)
> +				break;
> +
> +			index += burst_size;
> +			data += burst_size;
> +			size -= burst_size;
> +		} else {
> +			ret = get_registers(tp, index, type, size, data);
> +			if (ret < 0)
> +				break;
> +
> +			index += size;
> +			data += size;
> +			size = 0;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
> +		      u16 size, void *data, u16 type)
> +{
> +	int ret;
> +	u16 byteen_start, byteen_end, byte_en_to_hw;
> +	u16 burst_size = 512;
> +
> +	/* both size and index must be 4 bytes align */
> +	if ((size & 3) || !size || (index & 3) || !data)
> +		return -EINVAL;
> +
> +	if (index + size > 0xffff)
> +		return -EINVAL;
> +
> +	byteen_start = byteen & BYTE_EN_START_MASK;
> +	byteen_end = byteen & BYTE_EN_END_MASK;
> +
> +	byte_en_to_hw = byteen_start | (byteen_start << 4);
> +	ret = set_registers(tp, index, type | byte_en_to_hw, 4, data);
> +	if (ret < 0)
> +		return ret;
> +
> +	index += 4;
> +	data += 4;
> +	size -= 4;
> +
> +	if (size) {
> +		size -= 4;
> +
> +		while (size) {
> +			if (size > burst_size) {
> +				ret = set_registers(tp, index,
> +						    type | BYTE_EN_DWORD,
> +						    burst_size, data);
> +				if (ret < 0)
> +					return ret;
> +
> +				index += burst_size;
> +				data += burst_size;
> +				size -= burst_size;
> +			} else {
> +				ret = set_registers(tp, index,
> +						    type | BYTE_EN_DWORD,
> +						    size, data);
> +				if (ret < 0)
> +					return ret;
> +
> +				index += size;
> +				data += size;
> +				size = 0;
> +				break;

DTTO here

> +			}
> +		}
> +
> +		byte_en_to_hw = byteen_end | (byteen_end >> 4);
> +		ret = set_registers(tp, index, type | byte_en_to_hw, 4, data);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return ret;
> +}

[...]

> +u16 ocp_read_word(struct r8152 *tp, u16 type, u16 index)
> +{
> +	u32 data;
> +	__le32 tmp;
> +	u8 shift = index & 2;
> +
> +	index &= ~3;
> +
> +	generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);
> +
> +	data = __le32_to_cpu(tmp);
> +	data >>= (shift * 8);
> +	data &= 0xffff;

Can you please review the typecasts in the driver ? I don't think that a
lot of them are really necessary.

> +	return (u16)data;
> +}

[...]

> +static void rtl8152_nic_reset(struct r8152 *tp)
> +{
> +	int	i;
> +
> +	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CR, PLA_CR_RST);
> +
> +	for (i = 0; i < 1000; i++) {
> +		if (!(ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CR) & PLA_CR_RST))
> +			break;
> +
> +		udelay(400);
> +	}

So what exactly happens if this function fails 1000 times ? The NIC will not
be reset, but the code will proceed with whatever other actions?

> +}

[...]

> +static void rtl_disable(struct r8152 *tp)
> +{
> +	u32 ocp_data;
> +	int i;
> +
> +	ocp_data = ocp_read_dword(tp, MCU_TYPE_PLA, PLA_RCR);
> +	ocp_data &= ~RCR_ACPT_ALL;
> +	ocp_write_dword(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
> +
> +	rxdy_gated_en(tp, true);
> +
> +	for (i = 0; i < 1000; i++) {

You might want to pull this magic value, 1000, into a macro, since it's being
pretty repetitive in this driver. It seems to have some sort of significance.

> +		ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
> +		if ((ocp_data & FIFO_EMPTY) == FIFO_EMPTY)
> +			break;
> +
> +		mdelay(2);
> +	}
> +
> +	for (i = 0; i < 1000; i++) {
> +		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_TCR0) & TCR0_TX_EMPTY)
> +			break;
> +		mdelay(2);
> +	}
> +
> +	rtl8152_nic_reset(tp);
> +}

[...]

> +static int r8152_init(struct eth_device *eth, bd_t *bd)
> +{
> +	struct ueth_data	*dev = (struct ueth_data *)eth->priv;

ueth_data[SPACE]*dev please.

> +	struct r8152 *tp = (struct r8152 *)dev->dev_priv;
> +
> +	u8 speed;
> +	int timeout = 0;
> +#define TIMEOUT_RESOLUTION 50	/* ms */
> +#define PHY_CONNECT_TIMEOUT 5000

I'd suggest to use const variable instead to leverage the typechecking.

> +	int link_detected;
> +
> +	debug("** %s()\n", __func__);
> +
> +	do {
> +		speed = rtl8152_get_speed(tp);
> +
> +		link_detected = speed & LINK_STATUS;
> +		if (!link_detected) {
> +			if (timeout == 0)
> +				printf("Waiting for Ethernet connection... ");
> +			mdelay(TIMEOUT_RESOLUTION);
> +			timeout += TIMEOUT_RESOLUTION;
> +		}
> +	} while (!link_detected && timeout < PHY_CONNECT_TIMEOUT);
> +	if (link_detected) {
> +		tp->rtl_ops.enable(tp);
> +
> +		if (timeout != 0)
> +			printf("done.\n");
> +	} else {
> +		printf("unable to connect.\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int r8152_send(struct eth_device *eth, void *packet, int length)
> +{
> +	struct ueth_data *dev = (struct ueth_data *)eth->priv;
> +
> +	u32 opts1, opts2 = 0;
> +
> +	int err;
> +
> +	int actual_len;
> +	unsigned char msg[PKTSIZE + sizeof(struct tx_desc)];
> +	struct tx_desc *tx_desc = (struct tx_desc *)msg;
> +
> +#define USB_BULK_SEND_TIMEOUT 5000

DTTO here and in all the places where you use #define FOO in a function.

> +	debug("** %s(), len %d\n", __func__, length);
> +
> +	opts1 = length | TX_FS | TX_LS;
> +
> +	tx_desc->opts2 = cpu_to_le32(opts2);
> +	tx_desc->opts1 = cpu_to_le32(opts1);
> +
> +	memcpy(msg + sizeof(struct tx_desc), (void *)packet, length);
> +
> +	err = usb_bulk_msg(dev->pusb_dev,
> +				usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
> +				(void *)msg,
> +				length + sizeof(struct tx_desc),
> +				&actual_len,
> +				USB_BULK_SEND_TIMEOUT);
> +	debug("Tx: len = %zu, actual = %u, err = %d\n",
> +	      length + sizeof(struct tx_desc), actual_len, err);
> +
> +	return err;
> +}

[...]

> +/* Probe to see if a new device is actually an realtek device */
> +int r8152_eth_probe(struct usb_device *dev, unsigned int ifnum,
> +		      struct ueth_data *ss)
> +{
> +	struct usb_interface *iface;
> +	struct usb_interface_descriptor *iface_desc;
> +	int ep_in_found = 0, ep_out_found = 0;
> +	int i;
> +
> +	struct r8152 *tp;
> +
> +	/* let's examine the device now */
> +	iface = &dev->config.if_desc[ifnum];
> +	iface_desc = &dev->config.if_desc[ifnum].desc;
> +
> +	for (i = 0; r8152_dongles[i].vendor != 0; i++) {

Maybe you can use ARRAY_SIZE() instead and avoid having 0x00 as a terminating
entry in the array . What do you think ?

> +		if (dev->descriptor.idVendor == r8152_dongles[i].vendor &&
> +		    dev->descriptor.idProduct == r8152_dongles[i].product)
> +			/* Found a supported dongle */
> +			break;
> +	}
> +
> +	if (r8152_dongles[i].vendor == 0)
> +		return 0;
> +
> +	memset(ss, 0, sizeof(struct ueth_data));
> +
> +	/* At this point, we know we've got a live one */
> +	debug("\n\nUSB Ethernet device detected: %#04x:%#04x\n",
> +	      dev->descriptor.idVendor, dev->descriptor.idProduct);
> +
> +	/* Initialize the ueth_data structure with some useful info */
> +	ss->ifnum = ifnum;
> +	ss->pusb_dev = dev;
> +	ss->subclass = iface_desc->bInterfaceSubClass;
> +	ss->protocol = iface_desc->bInterfaceProtocol;
> +
> +	/* alloc driver private */
> +	ss->dev_priv = calloc(1, sizeof(struct r8152));
> +
> +	if (!ss->dev_priv)
> +		return 0;
> +
> +	/*
> +	 * We are expecting a minimum of 3 endpoints - in, out (bulk), and
> +	 * int. We will ignore any others.
> +	 */
> +	for (i = 0; i < iface_desc->bNumEndpoints; i++) {
> +		/* is it an BULK endpoint? */
> +		if ((iface->ep_desc[i].bmAttributes &
> +		     USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) {
> +			u8 ep_addr = iface->ep_desc[i].bEndpointAddress;
> +			if (ep_addr & USB_DIR_IN) {
> +				if (!ep_in_found) {

You can probably rewrite it this way

if (!ep_in_found && (ep_addr & USB_DIR_IN)) {
 do stuff ...
}

This might trim down the indentation, which looks really bad. But if it doesn't,
then don't worry about this.

> +					ss->ep_in = ep_addr &
> +						USB_ENDPOINT_NUMBER_MASK;
> +					ep_in_found = 1;
> +				}
> +			} else {
> +				if (!ep_out_found) {
> +					ss->ep_out = ep_addr &
> +						USB_ENDPOINT_NUMBER_MASK;
> +					ep_out_found = 1;
> +				}
> +			}
> +		}
> +
> +		/* is it an interrupt endpoint? */
> +		if ((iface->ep_desc[i].bmAttributes &
> +		    USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) {
> +			ss->ep_int = iface->ep_desc[i].bEndpointAddress &
> +				USB_ENDPOINT_NUMBER_MASK;
> +			ss->irqinterval = iface->ep_desc[i].bInterval;
> +		}
> +	}

[...]