On Friday, November 13, 2015 at 05:03:01 PM, Stephen Warren wrote:
> From: Ted Chen <tedc...@realtek.com>
Hi,
> This patch adds driver support for the Realtek RTL8152B/RTL8153 USB
> network adapters.
>
> Signed-off-by: Ted Chen <tedc...@realtek.com>
> [swarren, fixed a few compiler warnings]
> [swarren, with permission, converted license header to SPDX]
> [swarren, removed printf() spew during probe()]
> Signed-off-by: Stephen Warren <swar...@nvidia.com>
[...]
> new file mode 100644
> index 000000000000..7ab08bef4111
> --- /dev/null
> +++ b/drivers/usb/eth/r8152.c
> @@ -0,0 +1,3809 @@
> +/*
> + * Copyright (c) 2015 Realtek Semiconductor Corp. All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + *
> + * This product is covered by one or more of the following patents:
> + * US6,570,884, US6,115,776, and US6,327,625.
Why do we even care, does this have any significance ?
> + */
> +
> +#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"
> +
> +#define DRIVER_VERSION "v1.0 (2015/09/17)"
This macro is unused, please remove it.
> +#define PATENTS "This product is covered by one or more of the
> "
\
> + "following patents:\n" \
> + "\t\tUS6,570,884, US6,115,776, and US6,327,625.\n"
DTTO
[...]
> +/* OCP_EEE_CONFIG1 */
> +#define RG_TXLPI_MSK_HFDUP 0x8000
> +#define RG_MATCLR_EN 0x4000
> +#define EEE_10_CAP 0x2000
> +#define EEE_NWAY_EN 0x1000
> +#define TX_QUIET_EN 0x0200
> +#define RX_QUIET_EN 0x0100
> +#define sd_rise_time_mask 0x0070
> +#define sd_rise_time(x) (min(x, 7) << 4) /* bit 4 ~ 6 */
Should be min((x).....) , the x should be in parenthesis, please fix globally.
> +#define RG_RXLPI_MSK_HFDUP 0x0008
> +#define SDFALLTIME 0x0007 /* bit 0 ~ 2 */
> +
> +/* OCP_EEE_CONFIG2 */
> +#define RG_LPIHYS_NUM 0x7000 /* bit 12 ~ 15 */
> +#define RG_DACQUIET_EN 0x0400
> +#define RG_LDVQUIET_EN 0x0200
> +#define RG_CKRSEL 0x0020
> +#define RG_EEEPRG_EN 0x0010
> +
> +/* OCP_EEE_CONFIG3 */
> +#define fast_snr_mask 0xff80
> +#define fast_snr(x) (min(x, 0x1ff) << 7) /* bit 7 ~ 15 */
DTTO.
> +#define RG_LFS_SEL 0x0060 /* bit 6 ~ 5 */
> +#define MSK_PH 0x0006 /* bit 0 ~ 3 */
[...]
> +struct tally_counter {
> + __le64 tx_packets;
Shouldn't this be standard u64 ?
> + __le64 rx_packets;
> + __le64 tx_errors;
> + __le32 rx_errors;
> + __le16 rx_missed;
> + __le16 align_errors;
> + __le32 tx_one_collision;
> + __le32 tx_multi_collision;
> + __le64 rx_unicast;
> + __le64 rx_broadcast;
> + __le32 rx_multicast;
> + __le16 tx_aborted;
> + __le16 tx_underrun;
> +};
[...]
> +struct r8152;
This forward declaration can be removed.
> +struct r8152 {
> + struct usb_device *udev;
> + struct usb_interface *intf;
> + bool supports_gmii;
> +
> + struct rtl_ops {
> + void (*init)(struct r8152 *);
> + int (*enable)(struct r8152 *);
> + void (*disable)(struct r8152 *);
> + void (*up)(struct r8152 *);
> + void (*down)(struct r8152 *);
> + void (*unload)(struct r8152 *);
> + } rtl_ops;
> +
> + u32 coalesce;
> + u16 ocp_base;
> +
> + u8 version;
> +};
[...]
> +#define agg_buf_sz 2048
> +
> +/* local vars */
> +static int curr_eth_dev; /* index for name of next device detected */
Shouldn't we remove this for the sake of using DM ?
> +#define R8152_BASE_NAME "r8152"
> +
> +
> +struct r8152_dongle {
> + unsigned short vendor;
> + unsigned short product;
> +};
[...]
> +#define msleep(a) udelay(a * 1000)
That's called mdelay(), so please remove this and use mdelay()
> +#define BIT(nr) (1UL << (nr))
Don't we have a generic declaration of this bit macro ?
> +static
> +int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size, void
> *data) +{
> + int ret;
> + void *tmp;
> +
> + tmp = kmalloc(size, GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
Allocating the structure on stack would be faster than doing this
malloc-free dance, right? Please fix globally.
> + ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
> + RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> + value, index, tmp, size, 500);
> + if (ret < 0)
> + memset(data, 0xff, size);
> + else
> + memcpy(data, tmp, size);
> +
> + kfree(tmp);
> +
> + return ret;
> +}
[...]
> +static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
> + u16 size, void *data, u16 type)
> +{
> + int ret;
> + u16 byteen_start, byteen_end, byen;
> + u16 limit = 512;
> +
> + /* both size and indix must be 4 bytes align */
> + if ((size & 3) || !size || (index & 3) || !data)
> + return -EPERM;
> +
> + if ((u32)index + (u32)size > 0xffff)
Please review unnecessary typecast before next submission and remove them.
> + return -EPERM;
> +
> + byteen_start = byteen & BYTE_EN_START_MASK;
> + byteen_end = byteen & BYTE_EN_END_MASK;
> +
> + byen = byteen_start | (byteen_start << 4);
> + ret = set_registers(tp, index, type | byen, 4, data);
> + if (ret < 0)
> + goto error1;
> +
> + index += 4;
> + data += 4;
> + size -= 4;
[...]
> +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++) {
> + ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_OOB_CTRL);
> + if ((ocp_data & FIFO_EMPTY) == FIFO_EMPTY)
> + break;
> +
> + udelay(2000);
> + }
Is this a poor-man's attempt at implementing wait-for-bit sort of functionality?
If so, I'm concerned about the case where that bit is never set/unset, since
this case is not handled by the code.
Also, you can fix udelay(2000) to mdelay(2), but this sort of wait-for-bit
should prefferably use get_timer() instead.
> + for (i = 0; i < 1000; i++) {
> + if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_TCR0) & TCR0_TX_EMPTY)
> + break;
> + udelay(2000);
> + }
> +
> + rtl8152_nic_reset(tp);
> +}
[...]
> +static void r8152b_firmware(struct r8152 *tp)
> +{
> + if (tp->version == RTL_VER_01) {
> + int i;
> + static u8 pla_patch_a[] = {
> + 0x08, 0xe0, 0x40, 0xe0,
> + 0x78, 0xe0, 0x85, 0xe0,
[...]
I think it might be better to pull this firmware blob into either a separate
header file or into global variable. Also, it would make sense to reformat
this array such that there would be more elements on a line (8 or 16), so
that the declaration makes better use of horizontal space.
> + 0x00, 0x00, 0x02, 0xc6,
> + 0x00, 0xbe, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00 };
> +
> + rtl_clear_bp(tp);
> +
> + generic_ocp_write(tp, 0xf800, 0xff, sizeof(pla_patch_a2),
> + pla_patch_a2, MCU_TYPE_PLA);
> +
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xfc26, 0x8000);
> +
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xfc28, 0x17a5);
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xfc2a, 0x13ad);
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xfc2c, 0x184d);
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xfc2e, 0x01e1);
> + }
> +}
[...]
> +static void r8153_firmware(struct r8152 *tp)
> +{
> + if (tp->version == RTL_VER_03) {
> + r8153_clear_bp(tp);
> +
> + r8153_pre_ram_code(tp, 0x7000);
> + sram_write(tp, 0xb820, 0x0290);
> + sram_write(tp, 0xa012, 0x0000);
> + sram_write(tp, 0xa014, 0x2c04);
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xb438, 0x2c18);
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xb438, 0x2c45);
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xb438, 0x2c45);
DTTO here, a nice loop over an array would be helpful.
[...]
> + ocp_write_word(tp, MCU_TYPE_USB, 0xfc32, 0x0000);
> + ocp_write_word(tp, MCU_TYPE_USB, 0xfc34, 0x0000);
> + ocp_write_word(tp, MCU_TYPE_USB, 0xfc36, 0x0000);
> + ocp_write_word(tp, MCU_TYPE_USB, USB_BP_EN, 0x000c);
> + }
> +}
[...]
The beginning looked really crude, but the rest of the driver looked much
better then. Thanks!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
