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

[Marek Vasut <marex@denx.de>]

On Friday, November 13, 2015 at 05:03:01 PM, Stephen Warren wrote:
> From: Ted Chen <tedchen@realtek.com>

Hi,

> This patch adds driver support for the Realtek RTL8152B/RTL8153 USB
> network adapters.
> 
> 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()]
> Signed-off-by: Stephen Warren <swarren@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!