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
prev parent 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