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 <[email protected]> > > --- > > 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 [email protected] http://lists.infradead.org/mailman/listinfo/barebox
