Page MenuHomeFreeBSD

Microchip's LAN78XX driver for FreeBSD
ClosedPublic

Authored by arshankhanifar_gmail.com on Apr 23 2018, 2:01 PM.
Referenced Files
Unknown Object (File)
Mon, Mar 18, 10:46 PM
Unknown Object (File)
Tue, Mar 5, 6:26 PM
Unknown Object (File)
Tue, Mar 5, 6:26 PM
Unknown Object (File)
Tue, Mar 5, 6:26 PM
Unknown Object (File)
Tue, Mar 5, 6:26 PM
Unknown Object (File)
Tue, Mar 5, 6:26 PM
Unknown Object (File)
Tue, Mar 5, 6:19 PM
Unknown Object (File)
Tue, Mar 5, 6:19 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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Please align the constants in the header file

sys/dev/usb/net/if_lan78xx.c
5–6

Let's collapse these to one line.

arshankhanifar_gmail.com marked an inline comment as done.

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

sys/dev/usb/usbdevs
297

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

reverted it back to use SMC2 as the vendor id.

sys/dev/usb/net/if_lan78xx.c
4–5

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

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

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

Should be AN78XX_E2P_CMD_ADDR_MASK ?

arshankhanifar_gmail.com added inline comments.
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 marked 2 inline comments as done.

more style fixes, typo fixes, and comment fixes

sys/modules/usb/lan78xx/Makefile
35

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

removed incomplete PHY driver from makefile.

arshankhanifar_gmail.com added inline comments.
sys/modules/usb/lan78xx/Makefile
35

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.

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

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.

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

sys/dev/usb/net/if_lan78xx.c
996

FreeBSD style:

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

}

1182

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

1522

Empty lines??

This revision is now accepted and ready to land.Apr 30 2018, 8:57 PM
arshankhanifar_gmail.com added inline comments.
sys/dev/usb/net/if_lan78xx.c
1182

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
sys/dev/usb/net/if_lan78xx.c
996

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;
}
sys/dev/usb/net/if_lan78xx.c
1182

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.

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.

sys/dev/usb/net/if_lan78xx.c
1416

These all should be

/*
 * This form.
 */
1440

overly long line

1454

strange indentation

I guess muggle is right out :)

arshankhanifar_gmail.com marked 2 inline comments as done.

changed the file name, and variables. build passes.

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
1454

Sorry I can't see it. Where?

This revision was automatically updated to reflect the committed changes.