From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Sat, 14 Nov 2015 02:13:56 +0100 Subject: [U-Boot] [PATCH 2/2] usb: eth: add Realtek RTL8152B/RTL8153 driver In-Reply-To: <1447430581-8805-2-git-send-email-swarren@wwwdotorg.org> References: <1447430581-8805-1-git-send-email-swarren@wwwdotorg.org> <1447430581-8805-2-git-send-email-swarren@wwwdotorg.org> Message-ID: <201511140213.56176.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Friday, November 13, 2015 at 05:03:01 PM, Stephen Warren wrote: > From: Ted Chen Hi, > This patch adds driver support for the Realtek RTL8152B/RTL8153 USB > network adapters. > > Signed-off-by: Ted Chen > [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 [...] > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#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!