Re: [PATCH v2] usb: net: Add support for the Realtek RTL8152B/RTL8153

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hello Ahmad,

Thank you for review.

On Wed, Oct 20, 2021 at 09:25:09AM +0200, Ahmad Fatoum wrote:
> Hello Oleksij,
> 
> On 20.10.21 08:42, Oleksij Rempel wrote:
> > This adds support for the Realtek RTL8152B/RTL8153 ethernet converter chip.
> > This driver is based on U-Boot version v2021.10 with port to barebox
> > internal frameworks.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/usb/Kconfig    |    7 +
...
> > +static int get_registers(struct r8152 *tp, u16 value, u16 index, u16 size,
> > +			 void *data)
> > +{
> > +	void *buf;
> > +	int ret;
> > +
> > +	buf = xmalloc(size);
> 
> Should be dma_alloc to get proper alignment, as USB host drivers may map it
> for streaming DMA. You could allocate a 64 byte bounce buffer once at start
> and reuse it here instead of allocating on each control msg.

done.

> > +	if (!buf)
> > +		return -ENOMEM;
> 
> No need to check xmalloc return btw.

done

> > +
> > +	ret = usb_control_msg(tp->udev, usb_rcvctrlpipe(tp->udev, 0),
> > +			      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> > +			      value, index, buf, size, 500);
> > +	memcpy(data, buf, size);
> > +
> > +	free(buf);
> > +
> > +	return ret;
> > +}

...

> > +static int r8152_wait_for_bit(struct r8152 *tp, bool ocp_reg, u16 type,
> > +			      u16 index, const u32 mask, bool set,
> > +			      unsigned int timeout)
> > +{
> > +	u32 val;
> > +	u64 start;
> > +
> > +	start = get_time_ns();
> > +	do {
> > +		if (ocp_reg)
> > +			val = ocp_reg_read(tp, index);
> > +		else
> > +			val = ocp_read_dword(tp, type, index);
> > +
> > +		if (!set)
> > +			val = ~val;
> > +
> > +		if ((val & mask) == mask)
> > +			return 0;
> > +
> > +		mdelay(1);
> 
> That's a very short delay IMO. I am not sure, spamming that
> often actually produces better results. In __net_poll Sascha says:
> 
> "The timeout can't be arbitrarily small, 2ms is the smallest
> we can do with the 1ms USB frame size.". Does this apply
> here as well?

yes, done.

> > +	} while (!is_timeout(start, timeout * MSECOND));
> > +
> > +	debug("%s: Timeout (index=%04x mask=%08x timeout=%d)\n",
> > +	      __func__, index, mask, timeout);
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +static void r8152b_reset_packet_filter(struct r8152 *tp)
> > +{
> > +	u32 ocp_data;
> > +
> > +	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_FMC);
> > +	ocp_data &= ~FMC_FCR_MCU_EN;
> > +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_FMC, ocp_data);
> > +	ocp_data |= FMC_FCR_MCU_EN;
> > +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_FMC, ocp_data);
> > +}
> > +
> > +static void rtl8152_wait_fifo_empty(struct r8152 *tp)
> > +{
> > +	int ret;
> > +
> > +	ret = r8152_wait_for_bit(tp, 0, MCU_TYPE_PLA, PLA_PHY_PWR,
> > +				 PLA_PHY_PWR_TXEMP, 1, R8152_WAIT_TIMEOUT);
> > +	if (ret)
> > +		debug("Timeout waiting for FIFO empty\n");
> 
> Please use dev_dbg here and everywhere else, so it's immediately
> known where they originate from.

done

> > +
> > +	ret = r8152_wait_for_bit(tp, 0, MCU_TYPE_PLA, PLA_TCR0,
> > +				 TCR0_TX_EMPTY, 1, R8152_WAIT_TIMEOUT);
> > +	if (ret)
> > +		debug("Timeout waiting for TX empty\n");
> > +}
> > +
...
> > +static int r8152_init_common(struct r8152 *tp, struct usbnet *dev)
> > +{
> > +	bool timeout = false;
> > +	int link_detected;
> > +	u64 start;
> > +	u8 speed;
> > +
> > +	debug("** %s()\n", __func__);
> > +
> > +	printf("Waiting for Ethernet connection... ");
> > +	start = get_time_ns();
> > +	do {
> > +		speed = rtl8152_get_speed(tp);
> > +
> > +		link_detected = speed & LINK_STATUS;
> > +		if (!link_detected) {
> > +			mdelay(TIMEOUT_RESOLUTION);
> > +			if (is_timeout(start, PHY_CONNECT_TIMEOUT * MSECOND))
> > +				timeout = true;
> 
> return an error code?

done

> > +		}
> > +	} while (!link_detected && !timeout);
> > +
> > +	if (link_detected) {
> > +		tp->rtl_ops.enable(tp);
> > +
> > +		if (!timeout)
> > +			printf("done.\n");
> 
> timeout == true => link_detected == false, so this condition is
> never false. Just early exit above.

done

> > +	} else {
> > +		printf("unable to connect.\n");
> 
> dev_warn, so it includes device name and is written to log (dmesg/pstore).

done

> > +	}
> > +
> > +	return 0;
> 
> You check this code although it's always zero, so you never act on
> the unabel to connect.

fixed

> > +static int r8153_pre_ram_code(struct r8152 *tp, u16 patch_key)
> > +{
> > +	u16 data;
> > +	int i;
> > +
> > +	data = ocp_reg_read(tp, 0xb820);
> > +	data |= 0x0010;
> > +	ocp_reg_write(tp, 0xb820, data);
> > +
> > +	for (i = 0, data = 0; !data && i < 5000; i++) {
> > +		mdelay(2);
> 
> That's up to 10 seconds

converted to is_timeout()

> > +		data = ocp_reg_read(tp, 0xb800) & 0x0040;
> > +	}
> > +
> > +	sram_write(tp, 0x8146, patch_key);
> > +	sram_write(tp, 0xb82e, 0x0001);
> > +
> > +	return -EBUSY;
> 
> This is ignored. At least a message why it took 10 seconds
> would be nice.

hm, this return value makes no sense. I added warning and converted
function to void.

> > +}
> > +
> > +static int r8153_post_ram_code(struct r8152 *tp)
> > +{
> > +	u16 data;
> > +
> > +	sram_write(tp, 0x0000, 0x0000);
> > +
> > +	data = ocp_reg_read(tp, 0xb82e);
> > +	data &= ~0x0001;
> > +	ocp_reg_write(tp, 0xb82e, data);
> > +
> > +	sram_write(tp, 0x8146, 0x0000);
> > +
> > +	data = ocp_reg_read(tp, 0xb820);
> > +	data &= ~0x0010;
> > +	ocp_reg_write(tp, 0xb820, data);
> > +
> > +	ocp_write_word(tp, MCU_TYPE_PLA, PLA_OCP_GPHY_BASE, tp->ocp_base);
> > +
> > +	return 0;
> > +}
> > +
> > +static void r8153_wdt1_end(struct r8152 *tp)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < 104; i++) {
> > +		if (!(ocp_read_byte(tp, MCU_TYPE_USB, 0xe404) & 1))
> > +			break;
> > +		mdelay(2);

converted to is_timeout()

> > +	}
> > +}
> > +
> > +void r8152b_firmware(struct r8152 *tp)
> > +{
> > +	int i;
> > +
 
Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox