On Wednesday, November 25, 2015 at 06:30:54 AM, Ted Chen wrote:
> From: Ted Chen <tedchen at realtek.com>
>
> This patch adds driver support for the Realtek RTL8152B/RTL8153 USB
> network adapters.
>
> 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()]
> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>
> Changes for v2: Modified by Marek's comments.
> - Remove pattern informations.
> - Don't allocate & free when read/write register.
> - relpace udelay to mdelay.
> - pull firmware into global variable.
> - code review.
The changelog should go into the diffstat part, otherwise it will be picked and
become part of the commit message. We don't want that.
I only have nitpicks below :)
> Signed-off-by: Ted Chen <tedc...@realtek.com>
> ---
> drivers/usb/eth/Makefile | 1 +
> drivers/usb/eth/r8152.c | 3099
> +++++++++++++++++++++++++++++++++++++++++++ drivers/usb/eth/usb_ether.c |
> 8 +
> include/usb_ether.h | 6 +
> 4 files changed, 3114 insertions(+)
> create mode 100644 drivers/usb/eth/r8152.c
The changelog should go here.
> diff --git a/drivers/usb/eth/Makefile b/drivers/usb/eth/Makefile
> index c92d2b0..74f5f87 100644
> --- a/drivers/usb/eth/Makefile
> +++ b/drivers/usb/eth/Makefile
[...]
> +#define DRIVER_VERSION "v1.0 (2015/11/24)"
I don't think this is really relevant information.
> +#define R8152_PHY_ID 32
[...]
> +/* OCP_EEE_CONFIG3 */
> +#define fast_snr_mask 0xff80
> +#define fast_snr(x) (min(x, 0x1ff) << 7) /* bit 7 ~ 15 */
Please add parenthesis around the x -- min((x), ...
> +#define RG_LFS_SEL 0x0060 /* bit 6 ~ 5 */
> +#define MSK_PH 0x0006 /* bit 0 ~ 3 */
[...]
> +
> +#define msleep(a) mdelay(a)
Please just drop this.
> +static u8 r8152b_pla_patch_a[] = {
> + 0x08, 0xe0, 0x40, 0xe0, 0x78, 0xe0, 0x85, 0xe0,
Please add space after comma to make it consistent. Fix globally.
> + 0x5d, 0xe1, 0xa1, 0xe1, 0xa3, 0xe1, 0xab, 0xe1,
[...]
> +static u16 r8152b_ram_code1[] = {
> + 0x9700, 0x7fe0, 0x4c00, 0x4007, 0x4400, 0x4800, 0x7c1f, 0x4c00,
Please drop the tab and add space. Fix globally.
> + 0x5310, 0x6000, 0x7c07, 0x6800, 0x673e, 0x0000, 0x0000, 0x571f,
[...]
> +static int generic_ocp_read(struct r8152 *tp, u16 index, u16 size,
> + void *data, u16 type)
> +{
> + u16 limit = 64;
> + int ret = 0;
> +
> + /* both size and indix must be 4 bytes align */
> + if ((size & 3) || !size || (index & 3) || !data)
> + return -EPERM;
> +
> + if ((u32)index + (u32)size > 0xffff)
Are the casts here needed ?
> + return -EPERM;
> +
> + while (size) {
> + if (size > limit) {
> + ret = get_registers(tp, index, type, limit, data);
> + if (ret < 0)
> + break;
> +
> + index += limit;
> + data += limit;
> + size -= limit;
> + } else {
> + ret = get_registers(tp, index, type, size, data);
> + if (ret < 0)
> + break;
> +
> + index += size;
> + data += size;
> + size = 0;
> + break;
> + }
> + }
> +
> + 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)
> + 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;
> +
> + if (size) {
> + size -= 4;
> +
> + while (size) {
> + if (size > limit) {
> + ret = set_registers(tp, index,
> + type | BYTE_EN_DWORD,
> + limit, data);
> + if (ret < 0)
> + goto error1;
> +
> + index += limit;
> + data += limit;
> + size -= limit;
Cannot the branches of this code be rewritten in a more compact way ?
It seems you're only decrementing two variables by a different factor, no?
> + } else {
> + ret = set_registers(tp, index,
> + type | BYTE_EN_DWORD,
> + size, data);
> + if (ret < 0)
> + goto error1;
> +
> + index += size;
> + data += size;
> + size = 0;
> + break;
> + }
> + }
[...]
> +static u8 ocp_read_byte(struct r8152 *tp, u16 type, u16 index)
> +{
> + u32 data;
> + __le32 tmp;
> + u8 shift = index & 3;
> +
> + index &= ~3;
> +
> + generic_ocp_read(tp, index, sizeof(tmp), &tmp, type);
> +
> + data = __le32_to_cpu(tmp);
> + data >>= (shift * 8);
> + data &= 0xff;
> +
> + return (u8)data;
The cast is not needed, you might want to check this al around the place.
> +}
[...]
> +static void r8153_firmware(struct r8152 *tp)
> +{
> + int i;
> +
> + if (tp->version == RTL_VER_03) {
> + r8153_clear_bp(tp);
> +
> + r8153_pre_ram_code(tp, 0x7000);
> +
> + for (i = 0; i < ARRAY_SIZE(r8153_ram_code_a); i = i+2)
> + ocp_write_word(tp, MCU_TYPE_PLA,
> + r8153_ram_code_a[i],
> + r8153_ram_code_a[i+1]);
> +
> + r8153_post_ram_code(tp);
> + } else if (tp->version == RTL_VER_04) {
> + r8153_pre_ram_code(tp, 0x7001);
> +
> + for (i = 0; i < ARRAY_SIZE(r8153_ram_code_bc); i = i+2)
> + ocp_write_word(tp, MCU_TYPE_PLA,
> + r8153_ram_code_bc[i],
> + r8153_ram_code_bc[i+1]);
> +
> + r8153_post_ram_code(tp);
> +
> + r8153_wdt1_end(tp);
> + r8153_clear_bp(tp);
> +
> + ocp_write_word(tp, MCU_TYPE_USB, USB_BP_EN, 0x0000);
> + generic_ocp_write(tp, 0xf800, 0xff,
> + sizeof(r8153_usb_patch_b),
> + r8153_usb_patch_b, MCU_TYPE_USB);
> +
> + for (i = 0; i < ARRAY_SIZE(r8153_usb_patch_b_bp); i = i+2)
i += 2 is simpler, please fix globally
> + ocp_write_word(tp, MCU_TYPE_USB,
> + r8153_usb_patch_b_bp[i],
> + r8153_usb_patch_b_bp[i+1]);
> +
> + if (!(ocp_read_word(tp, MCU_TYPE_PLA, 0xd38e) & BIT(0))) {
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xd38c, 0x0082);
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xd38e, 0x0082);
> + }
> +
> + ocp_write_word(tp, MCU_TYPE_PLA, PLA_BP_EN, 0x0000);
> + generic_ocp_write(tp, 0xf800, 0xff,
> + sizeof(r8153_pla_patch_b),
> + r8153_pla_patch_b, MCU_TYPE_PLA);
> +
> + for (i = 0; i < ARRAY_SIZE(r8153_pla_patch_b_bp); i = i+2)
> + ocp_write_word(tp, MCU_TYPE_PLA,
> + r8153_pla_patch_b_bp[i],
> + r8153_pla_patch_b_bp[i+1]);
> +
> + ocp_write_word(tp, MCU_TYPE_PLA, 0xd388, 0x08ca);
[...]
Otherwise looks OK, thanks!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
