mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Oleksij Rempel <o.rempel@pengutronix.de>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH v2] usb: net: Add support for the Realtek RTL8152B/RTL8153
Date: Wed, 20 Oct 2021 12:32:53 +0200	[thread overview]
Message-ID: <20211020103253.GA8366@pengutronix.de> (raw)
In-Reply-To: <f8c19e10-df34-fc18-e7fd-d8bbaaaaf46b@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


      reply	other threads:[~2021-10-20 10:34 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-20  6:42 Oleksij Rempel
2021-10-20  7:25 ` Ahmad Fatoum
2021-10-20 10:32   ` Oleksij Rempel [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211020103253.GA8366@pengutronix.de \
    --to=o.rempel@pengutronix.de \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox