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 at 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 at 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;}- }
[...]