Page MenuHomeFreeBSD

Microchip's LAN78XX driver for FreeBSD
ClosedPublic

Authored by arshankhanifar_gmail.com on Apr 23 2018, 2:01 PM.

Details

Summary

This is a driver for Microchip's LAN78xx family.

What's left to do:

  • RX/TX Checksum Offloading: most of the code that handles RX checksum offloading is there, it works with ICMP packets, but it seems to break TCP/UDP packets.
  • PHY Driver: as of now this driver is using the generic ukphy driver, but an additional lan78xxphy would be more optimal for the chip (I think).
  • Perfect Address Filtering and Hash Address Filtering: the code for this is provided, but I haven't tested whether the filtering actually works yet.
  • VLAN Tag Filtering: not done yet.
  • USB Interrupt Endpoints: they're enabled but not used yet, the driver polls on the MII status, similar to the if_smsc driver so usb interrupts were not really needed. It would be nice to have though.
  • Wake Up and Suspend: Linux seems to have some infrastructure for handling this, I saw it being used in their driver. It would be really nice to add wake up and suspend capability to FreeBSD as well.
  • Makefile: The current Makefile provided works for building the driver, but I believe in order to get this to be built with buildkernel, there needs to be some more changes made to some Makefiles in the upper directories.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Please align the constants in the header file

sys/dev/usb/net/if_lan78xx.c
4–5 ↗(On Diff #41761)

Let's collapse these to one line.

emaste added subscribers: manu, db.Apr 24 2018, 7:29 PM
arshankhanifar_gmail.com marked an inline comment as done.

style fixes, removed all the TODOs in the code, and added a REMAINING FEATURES section.

diff wrt the base of this project.

emaste added inline comments.Apr 27 2018, 5:43 PM
sys/dev/usb/usbdevs
297 ↗(On Diff #41932)

These are ordered by ID it seems

Turns out SMC2 is the old Microchip. So I used that instead. We need to update the vendor description though! SMC2 doesn't represent microchip, which is the current company that makes micro things. :P

arshankhanifar_gmail.com marked an inline comment as done.Apr 27 2018, 6:19 PM

reverted it back to use SMC2 as the vendor id.

emaste added inline comments.Apr 27 2018, 7:38 PM
sys/dev/usb/net/if_lan78xx.c
4–5 ↗(On Diff #41935)

Collapse to one line.
* Copyright (C) 2012 Ben Gray <bgray@freebsd.org>.

38 ↗(On Diff #41935)

Microchip LAN78XX USB-Ethernet driver
and link should probably go to https://www.microchip.com/wwwproducts/en/LAN7800 (i.e., to the direct product page).

43 ↗(On Diff #41935)

I'm not sure that's true - it's based on a number of different things, but is probably most closely based on the if_smsc driver?

47 ↗(On Diff #41935)

probably "UNIMPLEMENTED FEATURES" or such

56 ↗(On Diff #41935)

Pi

Since the R-Pi3B+ is the primary use case for this driver right now we should mention what happens today (i.e., do we chose a random address? should the user provide one manually? etc.)

103 ↗(On Diff #41935)

This seems like a strange #define

159 ↗(On Diff #41935)
/*
 * Multi-line comments should be
 * like this.
 */

i.e. full sentences and begin and end are the /* and */ on their own.

202 ↗(On Diff #41935)

Don't need the #if guards, all supported versions of FreeBSD are > 1000000.

275 ↗(On Diff #41935)

Remove these blank lines.

sys/dev/usb/net/if_lan78xxreg.h
29 ↗(On Diff #41935)

Ethernet

31 ↗(On Diff #41935)

probably 'taken' rather than 'brought'

32 ↗(On Diff #41935)

maybe from the Linux driver (lan78xx.h) so there's a direct reference to the filename

sys/modules/usb/lan78xx/Makefile
4 ↗(On Diff #41935)

old (c) statement. probably we should just omit the copyright and license from the makefile.

andreast added inline comments.
sys/dev/usb/net/if_lan78xxreg.h
125 ↗(On Diff #41935)

Should be AN78XX_E2P_CMD_ADDR_MASK ?

arshankhanifar_gmail.com marked 14 inline comments as done.Apr 30 2018, 2:49 PM
arshankhanifar_gmail.com added inline comments.
sys/dev/usb/net/if_lan78xx.c
103 ↗(On Diff #41935)

in the if_smsc driver this is USB_DEBUG_VAR. I think there are tunables that enable debug mode for the driver.

103 ↗(On Diff #41935)

I changed it to USB_DEBUG_VAR to be the same as if_smsc driver

sys/dev/usb/net/if_lan78xxreg.h
125 ↗(On Diff #41935)

yeah I must've made a mistake when fixing the tabs. I'll run a build before every commit to catch mistakes like this from now on.

arshankhanifar_gmail.com updated this revision to Diff 42002.EditedApr 30 2018, 2:50 PM
arshankhanifar_gmail.com marked 2 inline comments as done.

more style fixes, typo fixes, and comment fixes

arshankhanifar_gmail.com marked 3 inline comments as done.Apr 30 2018, 2:51 PM

More fixes, informally discussed with @emaste

andreast added inline comments.Apr 30 2018, 6:48 PM
sys/modules/usb/lan78xx/Makefile
35 ↗(On Diff #41935)

Is this lan78phy.c needed, if so, where is it?

removed incomplete PHY driver from makefile.

arshankhanifar_gmail.com marked an inline comment as done.Apr 30 2018, 7:37 PM
arshankhanifar_gmail.com added inline comments.
sys/modules/usb/lan78xx/Makefile
35 ↗(On Diff #41935)

I was working on a PHY driver in parallel to this. Currently this driver resolves to ukphy driver. I've removed it. Thanks for pointing this out.

arshankhanifar_gmail.com marked 2 inline comments as done.Apr 30 2018, 7:38 PM

removed the FDT includes, as they're for reading the MAC address from RPIs and are not implemented yet.

Added a few subscribers with USB network experience

This is the glue needed to build as a ko. If you add it to the KERNCONF file like 'device if_lan78xx # PI3b+ USB NIC' then you build it inline.

Index: sys/conf/files

  • sys/conf/files (revision 333092)

+++ sys/conf/files (working copy)
@@ -3317,6 +3317,7 @@
dev/usb/net/if_cue.c optional cue
dev/usb/net/if_ipheth.c optional ipheth
dev/usb/net/if_kue.c optional kue
+dev/usb/net/if_lan78xx.c optional if_lan78xx
dev/usb/net/if_mos.c optional mos
dev/usb/net/if_rue.c optional rue
dev/usb/net/if_smsc.c optional smsc

Index: sys/modules/usb/Makefile

  • sys/modules/usb/Makefile (revision 333092)

+++ sys/modules/usb/Makefile (working copy)
@@ -47,7 +47,7 @@
SUBDIR += ${_dwc_otg} ehci ${_musb} ohci uhci xhci ${_uss820dci} ${_at91dci} \

	  ${_atmegadci} ${_avr32dci} ${_rsu} ${_rsufw} ${_saf1761otg}

SUBDIR += ${_rum} ${_run} ${_runfw} ${_uath} upgt usie ural ${_zyd} ${_urtw}
-SUBDIR += atp cfumass uhid ukbd ums udbp ufm uep wmt wsp ugold uled
+SUBDIR += atp cfumass lan78xx uhid ukbd ums udbp ufm uep wmt wsp ugold uled
SUBDIR += ucom u3g uark ubsa ubser uchcom ucycom ufoma uftdi ugensa uipaq ulpt \

	  umct umcs umodem umoscom uplcom uslcom uvisor uvscom

SUBDIR += udl

imp added a comment.Apr 30 2018, 8:29 PM

lan78xx isn't a very FreeBSD-like device name: It doesn't start with 'u' like most other usb lan drivers and we usually don't name them so directly. History has shown that this driver will either (a) be used for a lan79<mumble> part or the lan7850, for example, will need a new driver. You don't have to rename all the variables / functions, but please consider selecting a different name,

In D15168#321360, @imp wrote:

(a) be used for a lan79<mumble>

That's already the case - we developed this driver to support the USB-Ethernet on the Raspberry Pi 3B+ which has a LAN7515.

hselasky accepted this revision.Apr 30 2018, 8:57 PM

Driver seems to follow the yellow brick road of the existing USB ethernet drivers. Some comments added.

sys/dev/usb/net/if_lan78xx.c
995 ↗(On Diff #42014)

FreeBSD style:

switch<space>()...{
case XXX:

}

1181 ↗(On Diff #42014)

There should be a check for every usbd_copy_out() that the offset is valid and contains at least XXX bytes of data.

1521 ↗(On Diff #42014)

Empty lines??

This revision is now accepted and ready to land.Apr 30 2018, 8:57 PM
arshankhanifar_gmail.com marked 3 inline comments as done.May 1 2018, 12:41 AM
arshankhanifar_gmail.com added inline comments.
sys/dev/usb/net/if_lan78xx.c
1181 ↗(On Diff #42014)

Each rx_cmd_* is 4 bytes. How can I check whether they contain 4 bytes of data? What if their value is intentionally zero?

arshankhanifar_gmail.com marked an inline comment as done.

Added error checking for offset in mii read callback.

This revision now requires review to proceed.May 1 2018, 12:42 AM
emaste added inline comments.May 1 2018, 2:41 AM
sys/dev/usb/net/if_lan78xx.c
995 ↗(On Diff #42014)

i.e.

switch (usbd_get_speed(sc->sc_ue.ue_udev)) {
case USB_SPEED_SUPER:        
        burst_cap = LAN78XX_DEFAULT_BURST_CAP_SIZE/LAN78XX_SS_USB_PKT_SIZE;
        break;
case USB_SPEED_HIGH:        
        burst_cap = LAN78XX_DEFAULT_BURST_CAP_SIZE/LAN78XX_HS_USB_PKT_SIZE;
        break;
default:
        burst_cap = LAN78XX_DEFAULT_BURST_CAP_SIZE/LAN78XX_FS_USB_PKT_SIZE;
        break;
}
hselasky added inline comments.May 1 2018, 8:52 AM
sys/dev/usb/net/if_lan78xx.c
1181 ↗(On Diff #42014)

You need to do something like this:

if ((off + 4) > actlen)
goto tr_setup;

arshankhanifar_gmail.com marked 3 inline comments as done.

more accurate error checking, style fixes.

arshankhanifar_gmail.com planned changes to this revision.May 1 2018, 9:47 PM

What is the preferred name for this driver? I basically used LAN78XX because that was the naming scheme linux went with. It's kind of confusing as this driver is used for also LAN7515 on raspberry pi's. My suggestion is: if_umicro.c since it's for microchip, and it starts with a u like other FreeBSD's usb-to-ethernet drivers. However, that naming doesn't have any info about the model.

Looking at existing examples,

File Devices
if_aueADMtek AN986 Pegasus
if_axeASIX AX88x7x/760
if_axgeASIX AX88178A/AX88179 GigE
if_cdceUSB Communication Device Class
if_cueComputer Access Technology Corporation (CATC) USB-EL1210A
if_iphethApple iPhone / iPad tethered Ethernet
if_kueKawasaki LSI KL5KUSB101B
if_mosMoschip MCS7730/MCS7830/MCS7832
if_rueRealTek RTL8150
if_smscSMSC LAN9xxx
if_udavDavicom DM9601
if_ureRealTek RTL8152
if_urndisRemote NDIS
if_usieNo man page

So some ideas:

  • if_mchpge (MCHP = Microchip short form / stock ticker)
  • if_muge (Microchip USB Gigabit Ethernet)
  • if_mlge (Microchip LANxxxx Gigabit Ethernet)
  • if_mluge (Microchip LANxxxx USB Gigabit Ethernet)

After a brief IRC discussion it sounds like if_muge is the preferred option.
Aside, this exercise highlights that we need a man page for the new driver too.

emaste added inline comments.May 1 2018, 11:26 PM
sys/dev/usb/net/if_lan78xx.c
1415 ↗(On Diff #42058)

These all should be

/*
 * This form.
 */
1439 ↗(On Diff #42058)

overly long line

1453 ↗(On Diff #42058)

strange indentation

imp added a comment.May 1 2018, 11:28 PM

I guess muggle is right out :)

arshankhanifar_gmail.com marked 2 inline comments as done.

changed the file name, and variables. build passes.

hselasky accepted this revision.May 8 2018, 8:15 AM

Looks good to me.

This revision is now accepted and ready to land.May 8 2018, 8:15 AM

I added the pid in rS333489, as well as the pid for the LAN7801 (which Microchip sent me after the co-op term ended). I'll commit the driver with LAN7800 support only, then see if the 7801 "just works" or if we need additional changes.

style(9) issue - should not have trailing whitespace (tabs or spaces) at the end of the line
I will clean these up before commit

yeah I must've forgotten to do that sorry. Thanks!

sys/dev/usb/net/if_lan78xx.c
1453 ↗(On Diff #42058)

Sorry I can't see it. Where?

This revision was automatically updated to reflect the committed changes.