Add driver for RTL8153 based USB network dongles. [chromiumos/platform/depthcharge : master]

Pi-Hsun Shih (Gerrit)

unread,

Mar 19, 2018, 5:10:06 AM3/19/18

to chromium-...@chromium.org

Code is working, but need heavy cleanup.

View Change

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Pi-Hsun Shih (Gerrit)

unread,

Mar 20, 2018, 3:37:55 AM3/20/18

to Julius Werner, Hung-Te Lin

This change is ready for review.

View Change

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Hung-Te Lin (Gerrit)

unread,

Mar 20, 2018, 4:03:24 AM3/20/18

to Pi-Hsun Shih, Julius Werner

Wow, so fast! This in general looks good to me, but I think Julius will want to take a look.

Patch set 8:Code-Review +1

View Change

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Julius Werner (Gerrit)

unread,

Mar 20, 2018, 4:14:05 PM3/20/18

to Pi-Hsun Shih, Hung-Te Lin

This is some great work, thanks!

View Change

8 comments:

File src/drivers/net/r8152.c:
- Patch Set #8, Line 297: static int r8153_u1u2en(usbdev_t *dev, int enable)
  u1/u2/u3/u4 are low-power states. I don't think we want to enable those in firmware. Let's keep it as simple as possible.
- Patch Set #8, Line 334: uint16_t *val)
  Looks like you're never really using the value passed out from here, so you should probably get rid of that parameter.
- Patch Set #8, Line 334: static int r8153_phy_status(usbdev_t *dev, uint8_t desired, uint16_t *val)
  nit: name should probably reflect that this will wait on something (e.g. r8153_wait_for_phy_status())
- Patch Set #8, Line 363: if (ocp_write_word(dev, McuTypeUsb, UsbPowerCut, data))
  You're doing the read/modify/write on OCP words so often it's probably worth to write a wrapper for that, e.g.:
```
 int ocp_word_clrsetbits(usbdev_t *dev, uint16_t type, uint16_t index,
                         uint16_t clr_mask, uint16_t set_mask)
```
  (Maybe an ocp_reg_clrsetbits() as well.)
- Patch Set #8, Line 583: if (r8152_dev.version == RtlVersion04) {
  (This also looks related to suspend states and can probably be removed together with the U2/U3 stuff.)
- Patch Set #8, Line 727: static int rtl8152_mdio_write(NetDevice *dev, uint8_t loc, uint16_t val)
  nit: these wrappers seem a bit pointless?
- Patch Set #8, Line 821: if (ocp_write_byte(usb_dev, McuTypePla, PlaCrwecr,
  These next three seem odd. Why are you changing state when you just want to read the MAC address? Is this maybe something that should happen during init() instead (if we need it at all)?
- Patch Set #8, Line 846: { 0x13b1, 0x0041 },
  Shouldn't we copy the whole table supported by the Linux driver in here? Do we have reason to believe that the others wouldn't work? (Even if they're r8152 or r8153b, it's probably nicer to bind them to this driver and get a clean error message that they're not supported rather than just not enumerating them at all.)

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Ezequiel Garcia (Gerrit)

unread,

Mar 26, 2018, 10:40:34 AM3/26/18

to Pi-Hsun Shih, Hung-Te Lin, Julius Werner

This is great. Cherry-picking this commit and changing r8153_u2p3en (see the comment), I managed to TFTP a kernel using my dongle.

View Change

2 comments:

File src/drivers/net/r8152.c:
- Patch Set #8, Line 462: if (r8153_u2p3en(dev, 1))
  I'm not exactly sure what this is, but it's causing trouble. I had to change it to r8153_u2p3en(dev, 0), to avoid getting a "packet too large" error on tftpboot.

- Patch Set #8, Line 846: { 0x13b1, 0x0041 },

- Shouldn't we copy the whole table supported by the Linux driver in here? Do we have reason to believ […]
  I agree with this. And it probably makes sense to copy U-Boot's table https://github.com/trini/u-boot/blob/master/drivers/usb/eth/r8152.c#L1629.

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Ezequiel Garcia (Gerrit)

unread,

Mar 26, 2018, 5:21:57 PM3/26/18

to Pi-Hsun Shih, Hung-Te Lin, Julius Werner

Patch Set 8:
(2 comments)

This is great. Cherry-picking this commit and changing r8153_u2p3en (see the comment), I managed to TFTP a kernel using my dongle.

Actually, it seems I was too fast to say victory on my side. Cherry-picking this commit is not really enough. Some more configuration was needed (cherry-picking random code from U-Boot) to make this work.

I've managed to tftpboot a kernel, but the speed is super super slow, and it takes ages to receive a 12 MiB kernel :-(

View Change

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Pi-Hsun Shih (Gerrit)

unread,

Mar 28, 2018, 2:48:23 AM3/28/18

to Hung-Te Lin, Julius Werner, Ezequiel Garcia

Pi-Hsun Shih uploaded patch set #9 to this change.

View Change

Add driver for RTL8153 based USB network dongles.

BUG=b:70655060
TEST=Booted into the netboot payload using Linksys USB3GIGV1.
BRANCH=none

Change-Id: I1678e33512a943dd1ab747ee5757dae543d39b93
Signed-off-by: Pi-Hsun Shih <pih...@chromium.org>
---
M src/drivers/net/Makefile.inc
A src/drivers/net/r8152.c
A src/drivers/net/r8152.h
3 files changed, 1,085 insertions(+), 0 deletions(-)

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Pi-Hsun Shih (Gerrit)

unread,

Mar 28, 2018, 3:04:05 AM3/28/18

to Ezequiel Garcia, Hung-Te Lin, Julius Werner

Patch Set 8:

Patch Set 8:
(2 comments)
This is great. Cherry-picking this commit and changing r8153_u2p3en (see the comment), I managed to TFTP a kernel using my dongle.
Actually, it seems I was too fast to say victory on my side. Cherry-picking this commit is not really enough. Some more configuration was needed (cherry-picking random code from U-Boot) to make this work.
I've managed to tftpboot a kernel, but the speed is super super slow, and it takes ages to receive a 12 MiB kernel :-(

In my testing, the speed is about the same as the asix driver (15 seconds vs 12 seconds on a 14M kernel). Which USB dongle / device are you testing on?

View Change

9 comments:

File src/drivers/net/r8152.c:
- Patch Set #8, Line 297: uint32_t ocp_data;
  u1/u2/u3/u4 are low-power states. I don't think we want to enable those in firmware. […]
  Done
- Patch Set #8, Line 334: default:
  nit: name should probably reflect that this will wait on something (e.g. […]
  Done

- Patch Set #8, Line 334:

- Looks like you're never really using the value passed out from here, so you should probably get rid […]
  Done
- Patch Set #8, Line 363: {
  You're doing the read/modify/write on OCP words so often it's probably worth to write a wrapper for […]
  Done
- Patch Set #8, Line 462: if (sram_write(dev, Sram10mAmp2, 0x0208))

- I'm not exactly sure what this is, but it's causing trouble. […]

- I guess this is also low-power states related. I removed this section.
- Patch Set #8, Line 583: if (r8152_dev.version == RtlVersion05) {
  (This also looks related to suspend states and can probably be removed together with the U2/U3 stuff […]
  Done

- Patch Set #8, Line 727: }

- nit: these wrappers seem a bit pointless?
  These wrappers are here for the NetDevice interface.
- Patch Set #8, Line 821: /* TP-Link */
  These next three seem odd. […]
  Done

- Patch Set #8, Line 846:

- Shouldn't we copy the whole table supported by the Linux driver in here? Do we have reason to believ […]
  Done. I've merged both table together. Not sure why there's many different entries for the Lenovo section though.

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Pi-Hsun Shih (Gerrit)

unread,

Mar 28, 2018, 3:10:26 AM3/28/18

to Hung-Te Lin, Julius Werner, Ezequiel Garcia

Pi-Hsun Shih uploaded patch set #10 to this change.

View Change

Add driver for RTL8153 based USB network dongles.

BUG=b:70655060
TEST=Booted into the netboot payload using Linksys USB3GIGV1.
BRANCH=none

Change-Id: I1678e33512a943dd1ab747ee5757dae543d39b93
Signed-off-by: Pi-Hsun Shih <pih...@chromium.org>
---
M src/drivers/net/Makefile.inc
A src/drivers/net/r8152.c
A src/drivers/net/r8152.h

3 files changed, 1,062 insertions(+), 0 deletions(-)

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Julius Werner (Gerrit)

unread,

Mar 30, 2018, 5:50:47 PM3/30/18

to Pi-Hsun Shih, Ezequiel Garcia, Hung-Te Lin

Cool! I think review-wise this code is good to go now. Got a few more nits but I'm also okay with taking it as is if you don't want to bother anymore.

Patch set 10:Code-Review +2

View Change

5 comments:

File src/drivers/net/r8152.c:
- Patch Set #8, Line 727:

- These wrappers are here for the NetDevice interface.

- Yeah... I meant why rtl8152_mdio_write and r8152_mdio_write are separate functions. You could just use rtl8152_mdio_write everywhere, but I guess then you'd have to do a bunch of casts to get the NetDevice to pass in all the time. So on second thought this seems fine.

- Patch Set #8, Line 846:

- Done. I've merged both table together. […]
  In an ideal world, I think every vendor should just use the VID/PID from the chip they bought. Unfortunately, a lot of vendors seem to think they need to assign a different ID whenever they change the color of the plastic or something. So best we can do is just throw all IDs we know of in here. Don't worry about compatibility too much, having an ID that will fail to probe in here isn't really worse than not having the ID at all for depthcharge's purposes.
File src/drivers/net/r8152.c:
- Patch Set #10, Line 162: static int ocp_word_clrsetbits(usbdev_t *dev, uint16_t type, uint16_t index,
  nit: Should probably follow the convention for existing functions called clrsetbits, e.g. clrsetbits_le32 in libpayload/include/endian.h. They normally have a "clear mask" and a "set mask" argument and do write((read() & ~clear_mask) | set_mask), so that you can do things like clrsetbits(&register, mask_for_all_bits_in_multi_bit_field << field_shift, new_value_for_multi_bit_field << field_shift).
- Patch Set #10, Line 495: if (ocp_write_word(dev, McuTypePla, PlaFmc, data))
  nit: also use clrbits and setbits here? I don't think we care about the extra read...
- Patch Set #10, Line 658: printf("RTL8153b is not supported yet.");
  I noticed that the chip included on ServoV4 is of this type. Are you planning to add support for that later as well? I think a lot of our devs would really love you for that! (If not, let me know and I'll see if I can convince somebody else to do that... shouldn't be much more work after this, I hope.)

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Ezequiel Garcia (Gerrit)

unread,

Mar 30, 2018, 7:13:41 PM3/30/18

to Pi-Hsun Shih, ChromeOS Commit Bot, Julius Werner, Hung-Te Lin

I will try to re-test this next week, however I believe it might still fail for me.
I had to add a few more configuration. See https://gitlab.collabora.com/ezequiel/depthcharge/commit/c8f5fcf100e998ad79b0843681520b914745b5e4#1dd51462deb14698e4d0892deb11c4091456c154_662_675

View Change

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Pi-Hsun Shih (Gerrit)

unread,

Apr 1, 2018, 11:38:23 PM4/1/18

to Hung-Te Lin, Julius Werner, ChromeOS Commit Bot, Ezequiel Garcia

Pi-Hsun Shih uploaded patch set #11 to this change.

View Change

Add driver for RTL8153 based USB network dongles.

BUG=b:70655060
TEST=Booted into the netboot payload using Linksys USB3GIGV1.
BRANCH=none

Change-Id: I1678e33512a943dd1ab747ee5757dae543d39b93
Signed-off-by: Pi-Hsun Shih <pih...@chromium.org>
---
M src/drivers/net/Makefile.inc
A src/drivers/net/r8152.c
A src/drivers/net/r8152.h

3 files changed, 1,044 insertions(+), 0 deletions(-)

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Pi-Hsun Shih (Gerrit)

unread,

Apr 1, 2018, 11:38:33 PM4/1/18

to ChromeOS Commit Bot, Julius Werner, Ezequiel Garcia, Hung-Te Lin

View Change

3 comments:

File src/drivers/net/r8152.c:
- Patch Set #10, Line 162: static int ocp_word_clrsetbits(usbdev_t *dev, uint16_t type, uint16_t index,
  nit: Should probably follow the convention for existing functions called clrsetbits, e.g. […]
  Done

- Patch Set #10, Line 495: if (ocp_write_word(dev, McuTypePla, PlaFmc, data))
  nit: also use clrbits and setbits here? I don't think we care about the extra read...

- Done

- Patch Set #10, Line 658: printf("RTL8153b is not supported yet.");

- I noticed that the chip included on ServoV4 is of this type. […]
  Yes I do plan to add support for the other two, but think that I should probably get things reviewed before writing too much code. Also I only have a RTL8153 dongle to test.
  Good to know that ServoV4 is using this type. Since I do have a ServoV4 for testing, I probably would port driver for RTL8153b next.

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Julius Werner (Gerrit)

unread,

Apr 2, 2018, 6:41:20 PM4/2/18

to Pi-Hsun Shih, ChromeOS Commit Bot, Ezequiel Garcia, Hung-Te Lin

Patch set 11:Code-Review +2

View Change

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

Pi-Hsun Shih (Gerrit)

unread,

Apr 2, 2018, 11:07:22 PM4/2/18

to Julius Werner, ChromeOS Commit Bot, Ezequiel Garcia, Hung-Te Lin

Patch set 11:Verified +1Commit-Queue +1

View Change

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

ChromeOS bot (Gerrit)

unread,

Apr 3, 2018, 1:44:44 AM4/3/18

to Pi-Hsun Shih, Julius Werner, ChromeOS Commit Bot, Ezequiel Garcia, Hung-Te Lin

ChromeOS bot merged this change.

View Change

Add driver for RTL8153 based USB network dongles.

BUG=b:70655060
TEST=Booted into the netboot payload using Linksys USB3GIGV1.
BRANCH=none

Change-Id: I1678e33512a943dd1ab747ee5757dae543d39b93
Signed-off-by: Pi-Hsun Shih <pih...@chromium.org>

Reviewed-on: https://chromium-review.googlesource.com/961992
Reviewed-by: Julius Werner <jwe...@chromium.org>


---
M src/drivers/net/Makefile.inc
A src/drivers/net/r8152.c
A src/drivers/net/r8152.h
3 files changed, 1,044 insertions(+), 0 deletions(-)

To view, visit change 961992. To unsubscribe, or for help writing mail filters, visit settings.

ChromeOS bot (Gerrit)

unread,

Apr 3, 2018, 1:44:44 AM4/3/18

to Pi-Hsun Shih, Hung-Te Lin, Julius Werner, ChromeOS Commit Bot, Ezequiel Garcia

ChromeOS bot uploaded patch set #12 to the change originally created by Pi-Hsun Shih.

Reply all

Reply to author

Forward