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

[Marek Vasut <marex@denx.de>]

On Wednesday, November 25, 2015 at 06:30:54 AM, Ted Chen wrote:
> From: Ted Chen <tedchen@realtek.com>
> 
> 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>
> 
> 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 <tedchen@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!