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;
> + }
> + }
[...]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
