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