drivers: ethernet: smsc9118 driver (as used by mps2_an385) #11680

pfalcon · 2018-11-27T11:19:05Z

This is initial WIP on implementing driver for SMSC9220/SMSC9118/LAN9118 Ethernet chips, as available in ARM MPS2 board (BOARD=mps2_an385). The main purpose of this driver is to be used with QEMU emulation however, it's not tested on a real hardware (so less attention is paid to matters like cable detection, etc.)

NOTE to reviewers: as it stands now, this is very dirty code, you probably want to skip looking at it for now. This is posted to reference it in other tickets and discuss ways to brush up the code with @galak, the primary requester of this driver.

pfalcon · 2018-11-27T11:40:37Z

@galak: So, this now basically works, at least on the level of pings. There's a lot to do both re: functionality, and of course re: cleanup. I'd like to discuss high-level matter regarding the latter:

Use of code imported from mbed. The API of the C file (smsc9220_eth.c) is more or less adhoc, I already work it around. I would plan to get rid of it, and access hardware registers directly in the driver. It thus would be more like eth_enc28j60.c than those which use HALs. The header would stay, but would be patched.
Naming. The chips are apparently due to the company "SMSC" and where called SMSC9220, etc. It apparently was bought by Microchip, which renamed them to LAN9220, etc. Also, while MPS2 uses SMSC9220, QEMU actually emulated SMSC9118. The difference is in the things like checksum offloading, etc., something which we won't use. So I guess, it's more faithful to call the driver eth_smsc9118.c. For reference, in Linux it's called smsc911x.c IIRC.

@galak, so please let me know if you see something terribly wrong with the above, otherwise I'll gradually move it in that direction. Thanks.

codecov-io · 2018-11-27T12:03:24Z

Codecov Report

Merging #11680 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11680   +/-   ##
=======================================
  Coverage   53.78%   53.78%           
=======================================
  Files         242      242           
  Lines       27697    27697           
  Branches     6729     6729           
=======================================
  Hits        14896    14896           
  Misses       9997     9997           
  Partials     2804     2804

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f956a9...77ab2b4. Read the comment docs.

zephyrbot · 2018-11-29T19:59:57Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

pfalcon · 2018-11-29T20:01:12Z

Ok, so I decided to settle on naming it eth_smsc911x.c after all.

Functionality progresses well, pings now seem to be pretty reliable.

pfalcon · 2018-12-27T17:20:43Z

Updated to @tbursztyka's recently merged refactors. Pings work.

pfalcon · 2018-12-27T18:40:14Z

Update with qemu eth support, a-la #11138.

Now

zephyr-make BOARD=mps2_an385 CONF_FILE="prj.conf overlay-smsc911x.conf" run

works.

pfalcon · 2019-01-15T16:01:25Z

Ok, logging issues were due to enabling CONFIG_LOG_INPLACE_PROCESS, and now ticketed as #12494 . Logging workarounds were removed from this PR now, and it contains only the relevant stuff.

@galak, @jukkar, @tbursztyka : Please review.

tbursztyka

Minor comments, overall seems fine to me.

tbursztyka · 2019-01-15T16:08:39Z

drivers/ethernet/CMakeLists.txt

@@ -10,6 +10,7 @@ zephyr_sources_ifdef(CONFIG_ETH_DW		eth_dw.c)
 zephyr_sources_ifdef(CONFIG_ETH_E1000		eth_e1000.c)
 zephyr_sources_ifdef(CONFIG_ETH_ENC28J60	eth_enc28j60.c)
 zephyr_sources_ifdef(CONFIG_ETH_MCUX		eth_mcux.c)
+zephyr_sources_ifdef(CONFIG_ETH_SMSC911X	eth_smsc911x.c)


Merge this patch with previous one. Driver comes all file at once: code, Kconfig and built directives

Hmm, didn't notice we had it like that. Done.

tbursztyka · 2019-01-15T16:09:09Z

drivers/ethernet/eth_smsc911x_priv.h

@@ -0,0 +1,214 @@
+/* mbed Microcontroller Library


Merge this with next patch: driver code should come all at once.

tbursztyka · 2019-01-15T16:10:59Z

drivers/ethernet/eth_smsc911x.c

+	if (smsc_phy_regread(SMSC9220_PHY_ID1, &phyid1)) {
+		return -1;
+	}
+	if (smsc_phy_regread(SMSC9220_PHY_ID2, &phyid2)) {


have an empty line after the }, same below at line 262

tbursztyka · 2019-01-15T16:11:18Z

drivers/ethernet/eth_smsc911x.c

+	}
+
+	val |= 1 << 15;
+


you can remove that line since you use val in the if ()

you can remove that line since you use val in the if ()

Well, I'd say it's more symmetrical (read-modify-write) and easier to read the original way.

tbursztyka · 2019-01-15T16:11:47Z

drivers/ethernet/eth_smsc911x.c

+	/* threshold to defaults specified. */
+	SMSC9220->AFC_CFG = 0x006E3740;
+
+#if 0


Get rid of this code if it's never meant to be compiled.

Get rid of this code if it's never meant to be compiled.

So, this driver is written for hardware which actually exists, even though it's probably will be mostly used for QEMU emulated (== simplified) hardware, and that's how I tested it (I don't have access to MPS2 board). So, I wouldn't like to cut (or complicate) way to make it run well on real hw too. The original mbedOS driver had this code, I didn't convert it as QEMU doesn't emulate EEPROM, but whenever possible, I'd prefer to leave it in the driver even if commented out. I added comment why it is so.

So, I wouldn't like to cut (or complicate) way to make it run well on real hw too

Please do it then like this:

/* Not needed on QEMU, but may be needed on real hardware. */ if (!IS_ENABLED(CONFIG_QEMU_TARGET) && smsc_wait_eeprom()) { return -1; }

Please do it then like this:
if (!IS_ENABLED(CONFIG_QEMU_TARGET) && smsc_wait_eeprom())

That's confusing and misleading: not reading EEPROM doesn't depend on being QEMU_TARGET; smsc_wait_eeprom() doesn't exit.

It's just a TODO item/note to someone else who may want to get that driver running on the real HW, it should be good as it is.

That's confusing and misleading: not reading EEPROM doesn't depend on being QEMU_TARGET; smsc_wait_eeprom() doesn't exit.

Your comment is then misleading as it says that calling smsc_wait_eeprom() is not needed on QEMU. Anyway, lets remove this piece of code that is never going to be called, it is just confusing everybody. If we run this with real hardware, then suitable spells will be added here anyway.

Anyway, lets remove this piece of code

Ok, done.

tbursztyka · 2019-01-15T16:12:37Z

drivers/ethernet/eth_smsc911x.c

+	u32_t txcmd_a, txcmd_b;
+	u32_t tx_stat;
+
+	txcmd_a = (1/*is_first_segment*/ << 13) | (1/*is_last_segment*/ << 12)


You could have created dedicated macros for these bits.

You could have created dedicated macros for these bits.

I could, and actually I converted quite a bunch of raw numbers like that as they were in the original mbedOS. But not everything, and as this PR already spent a lot of time in works, with a lot of side-tracking, I kinda like it to pronounced done finally ;-)

tbursztyka · 2019-01-15T16:13:12Z

drivers/ethernet/eth_smsc911x.c

+
+static struct net_pkt *smsc_recv_pkt(u32_t pkt_size)
+{
+		u32_t rem_size;


all the code in this function seems be intended one tab too many

all the code in this function seems be intended one tab too many

Indeed, fixed.

pfalcon · 2019-01-15T16:59:31Z

Updated addressing @tbursztyka's comments.

nashif

Can we add this to the yaml file so it actually builds for this sample?

pfalcon · 2019-01-17T05:56:46Z

Can we add this to the yaml file so it actually builds for this sample?

@nashif, What exactly do you mean?

pfalcon · 2019-01-17T07:42:28Z

Can we add this to the yaml file so it actually builds for this sample?

Ok, I assume sample.yaml echo_server, which already has a bunch of overlay tests. Doing.

pfalcon · 2019-01-17T12:30:33Z

Ok, added sample.yaml change requested by @nashif and rebased on master. CI passes.

@tbursztyka, thanks for review, @jukkar, please have a look too.

jukkar

Looks good, couple of minor nitpicks

jukkar · 2019-01-17T13:03:58Z

drivers/ethernet/eth_smsc911x.c

+	/* threshold to defaults specified. */
+	SMSC9220->AFC_CFG = 0x006E3740;
+
+#if 0


So, I wouldn't like to cut (or complicate) way to make it run well on real hw too

Please do it then like this:

/* Not needed on QEMU, but may be needed on real hardware. */ if (!IS_ENABLED(CONFIG_QEMU_TARGET) && smsc_wait_eeprom()) { return -1; }

jukkar · 2019-01-17T13:06:13Z

drivers/ethernet/eth_smsc911x.c

+{
+	ARG_UNUSED(dev);
+
+	return ETHERNET_LINK_10BASE_T | ETHERNET_LINK_100BASE_T;


Please consider to add VLAN support later. This is not preventing merging now but it would be good to have that supported.

jukkar · 2019-01-17T13:06:58Z

drivers/ethernet/eth_smsc911x.c

+	struct device *dev = net_if_get_device(iface);
+	struct eth_context *context = dev->driver_data;
+	/* Must be static, netif has pointer stored */
+	static u8_t mac[6];


Why not add the mac string to eth_context instead?

Why not add the mac string to eth_context instead?

Done

jukkar · 2019-01-17T13:10:03Z

drivers/ethernet/eth_smsc911x.c

+	int res;
+	u16_t total_len = net_pkt_get_len(pkt);
+	u16_t sent = 0;
+	static u8_t tx_buf[1540] __aligned(4);


Why 1540 bytes here? For example in mcux, it allocates 1514 bytes buffer (max mtu + ethernet header size)

Why 1540 bytes here? For example in mcux, it allocates 1514

Fixed.

jukkar · 2019-01-17T13:11:09Z

drivers/ethernet/eth_smsc911x.c

+	SMSC9220->TX_DATA_PORT = txcmd_a;
+	SMSC9220->TX_DATA_PORT = txcmd_b;
+
+#define LINEARIZE


Why not have this as a Kconfig option, or just remove the unused branch if it is not working according to your comment.

Why not have this as a Kconfig option, or just remove the unused branch if it is not working according to your comment.

Definitely not ready to be a Kconfig option. I just implemented it initially w/o intermediate linearization step, but it got broken when @tbursztyka introduced arbitrary-sized fragments support, and I was reluctant to remove the original code, but did that now.

pfalcon · 2019-01-17T17:27:38Z

Pushed new version with @jukkar's comments addressed.

The board has SMSC LAN9220 (actually as an "IP core" in an FPGA-emulated SoC). The patch includes DTS bindings for this device. Signed-off-by: Paul Sokolovsky <[email protected]>

As emulated by QEMU. SMSC9118 is compatible with SMSC9220 as used in ARM MPS2 board, as well as SMSC9115/6/7/etc. devices. Portions of the code are based on mbedOS code from its targets/TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/drivers/smsc9220_eth.c eth_smsc9220_priv.h originally comes from Arm mbedOS file: targets/TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/drivers/smsc9220_eth.h augmented with struct & defines from: targets/TARGET_ARM_SSG/TARGET_CM3DS_MPS2/device/CM3DS.h and renamed as eth_smsc911x_priv.h to follow Zephyr conventions. Then, following changes applied: Changes to build under Zephyr, changes to use symbolic constants and field access helpers, typo fixes, etc. Signed-off-by: Paul Sokolovsky <[email protected]>

Board can be emulated in QEMU, so select QEMU_TARGET as required for various bits of "make run" magic to work. Signed-off-by: Paul Sokolovsky <[email protected]>

As can be used for example with BOARD=mps2_an385. Signed-off-by: Paul Sokolovsky <[email protected]>

Add overlays for commonly used socket samples: big_http_download and dumb_http_server. Can be used with: make BOARD=mps2_an385 CONF_FILE="prj.conf overlay-smsc911x.conf" Signed-off-by: Paul Sokolovsky <[email protected]>

mps2_an385 is enabled for networking -> default networking is SLIP -> SLIP selects UART_PIPE -> UART_PIPE requires UART_PIPE_ON_DEV defined -> undefined leads to error on building some samples. Fix all of these issues. Signed-off-by: Paul Sokolovsky <[email protected]>

By adding relevant test config to sample.yaml. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon · 2019-01-18T12:34:00Z

@jukkar: Please re-review.

pfalcon · 2019-01-18T14:25:36Z

@nashif, @jukkar : Your comments were addressed, please re-review.

pfalcon added area: Networking area: Ethernet labels Nov 27, 2018

pfalcon requested review from jukkar, nordic-krch and tbursztyka as code owners November 27, 2018 11:19

pfalcon requested a review from galak November 27, 2018 11:19

This was referenced Nov 28, 2018

logging: CONFIG_LOG_INPLACE_PROCESS is not synchronous #11633

Closed

Alleged multiple design and implementation issues with logging #11655

Closed

pfalcon force-pushed the smsc9220-pfalcon branch from e8a4152 to ff31356 Compare November 29, 2018 19:58

zephyrproject-rtos deleted a comment from zephyrbot Nov 29, 2018

pfalcon force-pushed the smsc9220-pfalcon branch 5 times, most recently from 3b047e6 to 7074eee Compare November 30, 2018 17:06

This was referenced Dec 1, 2018

L2 rework #10547

Merged

drivers/ethernet: Adapt stellaris driver to new L2 behaviour #11869

Merged

pfalcon force-pushed the smsc9220-pfalcon branch from 7074eee to d555799 Compare December 27, 2018 17:19

pfalcon requested a review from jhedberg as a code owner December 27, 2018 17:19

pfalcon force-pushed the smsc9220-pfalcon branch from d555799 to 699db4e Compare December 27, 2018 18:37

pfalcon force-pushed the smsc9220-pfalcon branch 4 times, most recently from 7e8b774 to 85215a4 Compare December 28, 2018 13:05

pfalcon removed the request for review from nordic-krch January 15, 2019 16:01

pfalcon force-pushed the smsc9220-pfalcon branch from 974bb6b to 7f1d04c Compare January 15, 2019 16:04

tbursztyka requested changes Jan 15, 2019

View reviewed changes

pfalcon force-pushed the smsc9220-pfalcon branch from 7f1d04c to c42c5ce Compare January 15, 2019 16:59

tbursztyka approved these changes Jan 15, 2019

View reviewed changes

nashif requested changes Jan 17, 2019

View reviewed changes

pfalcon force-pushed the smsc9220-pfalcon branch from c42c5ce to 548156b Compare January 17, 2019 07:43

jukkar requested changes Jan 17, 2019

View reviewed changes

pfalcon force-pushed the smsc9220-pfalcon branch from 548156b to abaf3a0 Compare January 17, 2019 17:27

pfalcon mentioned this pull request Jan 17, 2019

Refactored logger 'in place' mode #12406

Merged

pfalcon added 7 commits January 18, 2019 13:42

boards: arm: mps2_an385: Add DTS node for Ethernet controller

7e42237

The board has SMSC LAN9220 (actually as an "IP core" in an FPGA-emulated SoC). The patch includes DTS bindings for this device. Signed-off-by: Paul Sokolovsky <[email protected]>

boards: mps2_an385: Select QEMU_TARGET

90b35e9

Board can be emulated in QEMU, so select QEMU_TARGET as required for various bits of "make run" magic to work. Signed-off-by: Paul Sokolovsky <[email protected]>

samples: net: echo_server: Add overlay for eth_smsc911x

59c6956

As can be used for example with BOARD=mps2_an385. Signed-off-by: Paul Sokolovsky <[email protected]>

samples: sockets: Add eth_smsc911x overlays

332fc17

Add overlays for commonly used socket samples: big_http_download and dumb_http_server. Can be used with: make BOARD=mps2_an385 CONF_FILE="prj.conf overlay-smsc911x.conf" Signed-off-by: Paul Sokolovsky <[email protected]>

samples: echo_server: Test overlay-smsc911x.conf via sanitycheck

77ab2b4

By adding relevant test config to sample.yaml. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon force-pushed the smsc9220-pfalcon branch from abaf3a0 to 77ab2b4 Compare January 18, 2019 10:46

jukkar approved these changes Jan 18, 2019

View reviewed changes

pfalcon mentioned this pull request Jan 18, 2019

frdm_k64f: Sometimes, there may be 1000+ms roundtrip for pings and other Ethernet packets (instead of sub-ms) #3132

Closed

nashif approved these changes Jan 19, 2019

View reviewed changes

nashif merged commit 7117b73 into zephyrproject-rtos:master Jan 19, 2019

drivers: ethernet: smsc9118 driver (as used by mps2_an385) #11680

drivers: ethernet: smsc9118 driver (as used by mps2_an385) #11680

Uh oh!

Conversation

pfalcon commented Nov 27, 2018

Uh oh!

pfalcon commented Nov 27, 2018

Uh oh!

codecov-io commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zephyrbot commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfalcon commented Nov 29, 2018

Uh oh!

pfalcon commented Dec 27, 2018

Uh oh!

pfalcon commented Dec 27, 2018

Uh oh!

pfalcon commented Jan 15, 2019

Uh oh!

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Jan 15, 2019

Uh oh!

nashif left a comment

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Jan 17, 2019

Uh oh!

pfalcon commented Jan 17, 2019

Uh oh!

pfalcon commented Jan 17, 2019

Uh oh!

jukkar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Nov 27, 2018 •

edited

Loading

zephyrbot commented Nov 29, 2018 •

edited

Loading