Microchip's LAN78XX driver for FreeBSD

[arshankhanifar_gmail.com]

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.

[emaste]

Please align the constants in the header file

sys/dev/usb/net/if_lan78xx.c

4–5

Let's collapse these to one line.

[arshankhanifar_gmail.com]

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

[arshankhanifar_gmail.com]

diff wrt the base of this project.

[emaste]

sys/dev/usb/usbdevs

297

These are ordered by ID it seems

[arshankhanifar_gmail.com]

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]

reverted it back to use SMC2 as the vendor id.

[emaste]

sys/dev/usb/net/if_lan78xx.c

4–5

Collapse to one line.
* Copyright (C) 2012 Ben Gray <[e-mail]>.

38

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

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

probably "UNIMPLEMENTED FEATURES" or such

56

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

This seems like a strange #define

159

/*
 * Multi-line comments should be
 * like this.
 */

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

202

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

275

Remove these blank lines.

sys/dev/usb/net/if_lan78xxreg.h

29

Ethernet

31

probably 'taken' rather than 'brought'

32

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]

sys/dev/usb/net/if_lan78xxreg.h

125

Should be AN78XX_E2P_CMD_ADDR_MASK ?

[arshankhanifar_gmail.com]

sys/dev/usb/net/if_lan78xx.c

103

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

103

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

sys/dev/usb/net/if_lan78xxreg.h

125

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]

more style fixes, typo fixes, and comment fixes

[arshankhanifar_gmail.com]

More fixes, informally discussed with @emaste

[andreast]

sys/modules/usb/lan78xx/Makefile

35 ↗

(On Diff #41935)

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

[arshankhanifar_gmail.com]

removed incomplete PHY driver from makefile.

[arshankhanifar_gmail.com]

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]

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

[emaste]

Added a few subscribers with USB network experience

[andreast]

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]

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,

[emaste]

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]

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

FreeBSD style:

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

}

1181

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

1521

Empty lines??

[arshankhanifar_gmail.com]

sys/dev/usb/net/if_lan78xx.c

1181

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]

Added error checking for offset in mii read callback.

[emaste]

sys/dev/usb/net/if_lan78xx.c

995

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]

sys/dev/usb/net/if_lan78xx.c

1181

You need to do something like this:

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

[arshankhanifar_gmail.com]

more accurate error checking, style fixes.

[arshankhanifar_gmail.com]

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.

[emaste]

Looking at existing examples,

File	Devices
if_aue	ADMtek AN986 Pegasus
if_axe	ASIX AX88x7x/760
if_axge	ASIX AX88178A/AX88179 GigE
if_cdce	USB Communication Device Class
if_cue	Computer Access Technology Corporation (CATC) USB-EL1210A
if_ipheth	Apple iPhone / iPad tethered Ethernet
if_kue	Kawasaki LSI KL5KUSB101B
if_mos	Moschip MCS7730/MCS7830/MCS7832
if_rue	RealTek RTL8150
if_smsc	SMSC LAN9xxx
if_udav	Davicom DM9601
if_ure	RealTek RTL8152
if_urndis	Remote NDIS
if_usie	No 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)

[emaste]

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]

sys/dev/usb/net/if_lan78xx.c

1415

These all should be

/*
 * This form.
 */

1439

overly long line

1453

strange indentation

[imp]

I guess muggle is right out :)

[arshankhanifar_gmail.com]

changed the file name, and variables. build passes.

[• hselasky]

Looks good to me.

[emaste]

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.

[emaste]

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

[arshankhanifar_gmail.com]

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

sys/dev/usb/net/if_lan78xx.c

1453

Sorry I can't see it. Where?