stm32f4 eth peripheral driver #5643

lebrush · 2016-07-14T15:10:09Z

This is a working stm32f4 ethernet peripheral driver. It has been tested with lwIP and works like a charm.

I didn't create any header because I'm not sure how to abstract it. I see that there are possibly three public methods:

void eth_netdev_setup(netdev2_t *netdev)
void eth_phy_write(unsigned addr, unsigned reg, unsigned value)
unsigned eth_phy_read(unsigned addr, unsigned reg)

and I'm wondering if it would make sense to create a common header file as for all the peripheral drivers (periph/eth.h). The header file could expose these methods. Maybe something to do is to define as well a type eth_t for handling multiple of this devices, but I'm not sure that currently an MCU can support multiple eth peripherals.

Opinions? Feedback?

jnohlgard · 2016-07-16T09:40:14Z

What external hardware is required to use this with an STM32F4 Discovery? I guess it needs an Ethernet PHY?

kYc0o · 2016-07-16T12:40:05Z

Mmm maybe it's not especially for STM32F4Discovery but for STM32F4 in general, look at this board

lebrush · 2016-07-18T06:41:23Z

@gebart A PHY is not required. One could connect i.e. RMII-RMII as a bord-interconnect, allowing high data throughput. In case a PHY is used, all PHYs have common registers and vendor/model registers. This driver configures the common one regarding duplex and provides an standard interface to read/write registers from/to the PHY.

AFAIK all stm32fxy7 should have a ETH peripheral (maybe as the stm32fx12 as well, thanks @kYc0o!). The stm32f4discovery features a 407 :-)

This means: it's not board dependent and requires no extra specific hardware. It has been tested with and without PHY.

jnohlgard · 2016-07-18T07:46:15Z

boards/stm32f4discovery/Makefile.include

@@ -3,7 +3,7 @@ export CPU = stm32f4
 export CPU_MODEL = stm32f407vg

 # set default port depending on operating system
-PORT_LINUX ?= /dev/ttyUSB0


unrelated change?

I know is not related to the PR. I could open a different one if you prefer :-)

Rationale:
This board appears as ttyACM0 not as ttyUSB0 in my machine the same as most Nucleo boards I've tried. I'm not sure this is kernel or distribution dependent (tried on Ubuntu [kernel 3.13] and arch [kernel 4.4]) or were remaining of board copy-paste.

lebrush · 2016-08-03T07:59:38Z

Updated to use DMA to copy to TX buffers

lebrush · 2016-08-03T08:00:18Z

@OlegHahm @kaspar030 @gebart how should we proceed here?

kaspar030 · 2016-08-03T08:32:34Z

@lebrush The uncrustify and serial changes should go into a seperate cleanup PR.

lebrush · 2016-08-03T13:09:13Z

@kaspar030 done

lebrush · 2016-09-06T10:05:18Z

Added a generic Ethernet PHY interface: only read and write registers as well as common register definitions.

OlegHahm · 2016-09-23T16:16:21Z

@kaspar030, ping!

kaspar030

Nice PR, I have only minor comments. (and unfortunately no hardware to test this).

Do you think it makes sense to have this extra peripheral interface, instead of just using netdev2?

kaspar030 · 2016-09-24T20:34:17Z

cpu/stm32f4/periph/eth.c

+
+    switch (opt) {
+        case NETOPT_ADDRESS:
+            if (max_len >= ETHERNET_ADDR_LEN) {


Same here, please use assert.

kaspar030 · 2016-09-24T20:34:51Z

cpu/stm32f4/periph/eth.c

+
+    switch (opt) {
+        case NETOPT_ADDRESS:
+            if (max_len >= ETHERNET_ADDR_LEN) {


This would fail silently. Until now we used asserts for checking the length.

kaspar030 · 2016-09-24T20:39:03Z

cpu/stm32f4/periph/eth.c

+
+    int ret = 0, len = 0;
+    mutex_lock(&send_lock);
+    for (int i = 0; i < count && ret == 0; i++) {


doesn't eth_send() return != 0 even on success?

It returns the amount of bytes transmitted or -1 in case of error. Since netdev expect to return the amount of bytes transmitted is kind of practical to do it like this. In any case I added a condition in _send to make sure we don't ignore an error an pass it to netdev.

kaspar030 · 2016-09-24T20:45:17Z

cpu/stm32f4/periph/eth.c

+
+#include "cpu.h"
+#include "mutex.h"
+#include "cib.h"


What's this used for?

mmm... it's always nice to have unused includes... good catch, will remove ;-)

kaspar030 · 2016-09-24T20:49:01Z

cpu/stm32f4/periph/eth.c

+typedef struct eth_dma_desc edma_desc_t;
+
+/** Update pointer to next descriptor */
+#define _next_desc(x) x = (x->desc_next)


Would you mind removing this macro? We're trying to not use macros when possible.

kaspar030 · 2016-09-24T20:50:02Z

cpu/stm32f4/periph/eth.c

+    return 0;
+}
+
+static int eth_send(char *data, unsigned len)


can data be "const char *"?

lebrush · 2016-11-11T14:51:35Z

@kaspar030 sorry for the delay in the response. I think it makes sense to have it as a netdev2 and not as a separate device since it's OSI layer 2 device and integrates quite nicely with lwip :-)

lebrush · 2016-11-28T11:16:57Z

@kaspar030 @OlegHahm ping!

OlegHahm · 2016-12-08T18:25:07Z

Oops, sorry. Will investigate if I can access the hardware to test.

kYc0o · 2017-03-03T15:18:32Z

Can you rebase? I think some nucleo boards include this is it?

lebrush · 2017-03-06T13:45:31Z

@kYc0o done

aabadie · 2017-03-06T13:57:46Z

@kYc0o, I think you can try with a nucleo144-f207. If you want to test, I have one with me (but you'll need to adapt the config).

Tenderson · 2017-10-11T15:24:14Z

Hi @lebrush , @aabadie ,

I tried the latest RIOT master with the gnrc_networking example with Nucleo 144-f429, however the ifconfig is still not working. Shall i wait for merging, oder?

miri64 · 2017-11-14T14:03:41Z

It has been tested with lwIP and works like a charm.

Any reason why you didn't test it with GNRC? ;-)

lebrush · 2017-11-15T06:40:34Z

Not really... we needed a solution with ipv4 and lwip was the way to go. Unfortunately, I won't be able to drive this any further and I don't have hardware to test it. Someone else should take over.

smlng · 2018-01-12T10:19:36Z

needs adoption by someone, remove milestone until then.

MrKevinWeiss · 2018-04-11T12:10:44Z

I have some hardware with me (from home) so I can test it and maybe we can merge?

tcschmidt · 2018-06-01T22:29:45Z

@MrKevinWeiss did you try some testing? ... we should get this done after years, I guess ;)

MrKevinWeiss · 2018-06-03T18:17:15Z

I tried but many things need to get adapted after the years, it will require a bit of work to get it up to speed.

tcschmidt · 2018-06-03T18:21:40Z

@MrKevinWeiss can you name the things that require adaptation?
@lebrush would you take up then?

lebrush · 2018-06-03T18:35:34Z

Unfortunately, I've not the hardware anymore but I'll be happy to review it

smlng · 2018-06-04T06:56:35Z

@MrKevinWeiss if you have the hardware for testing I can help you with the code.

@lebrush is it okay for you that we take this over to get it merged?

MrKevinWeiss · 2018-06-04T08:53:53Z

@smlng Sure thing, unfortunately this is low in the priorities list but if you can get it done quick I will run it through the tests and close it off!

tcschmidt · 2018-06-11T10:03:46Z

@smlng guide code adaption first?

smlng · 2018-06-11T18:44:01Z

I already looked into, it's a bit of work to adapt this to the official DMA interface - but manageable.

tcschmidt · 2018-07-04T19:02:37Z

bit of work ,,, but manageable.

O.K. then good to manage @smlng

tcschmidt · 2018-08-21T17:06:07Z

@smlng ping

crest42 · 2018-12-15T00:49:49Z

Hi All,

I just ported this PR to work with the current riot master branch and to use it with the F7 nucleo family. (F767ZI to be specific). I was also able to use gnrc instead of lwip as a network stack and run a small test (icmp and udp echo).

I would like to adopt this PR since @lebrush is not working on it anymore. If this is fine for you please answer the following questions and i will try to push this further :)

What are the blockers? In the last messages there was something about an adaption to an official DMA Interface. Can i do something to help you there?
What is the "best" way to implement this as a network driver. As a peripheral driver as in the original work or as a netdev driver under drivers/ ?
How should i commit my work? Open a new PR or can i push my additional stuff on this PR somehow?
Does it make sense to add this network interface as a default interface for the nucleo boards with on board ethernet to make the usage easier?
Does it make sense to rewrite the hardcoded macaddr to something more dynamic?

What i have done so far:

Rework the example from lwip to gnrc
Update the driver to match the new vendor header definitions für nucleo boards
Update the driver to use iolist instead of iovec (a little bit hacky but i will fix this)
Add Support for f7 family
Testing on the f7 family

Regards Robin

crest42 · 2018-12-15T23:42:43Z

What is the "best" way to implement this as a network driver. As a peripheral driver as in the original work or as a netdev driver under drivers/ ?

By now it stays as a peripheral but i moved the code from the f4 speicific folder to the stm32_common folder

Does it make sense to rewrite the hardcoded macaddr to something more dynamic?

I just added a dynamic macaddr config using luid_get from the luid module in case the configured mac address starts with 0 in the first byte. (according to rfc7042 this region is reserved and thus should be safe to use)

MrKevinWeiss · 2018-12-17T08:45:37Z

@crest42 Very nice. I think I looked at this a while back and it ended up being a non-trivial rebase. I think @smlng discovered that too.

I think it would be nice to open a new PR (and give whatever credit/reference @lebrush) so that we could review it (especially if you have it working on current master).

smlng · 2018-12-17T09:32:20Z

@crest42 nice to hear that. As @MrKevinWeiss already mentioned, I tried my luck with this one, too ... however the main problem was, that I didn't have the board originally used here for testing.

It seems there are several differences between the various STM Nucleos with ethernet, i.e. pinout and DMA stream used.

Anyway, you can either open a PR or point us your branch so we can have look at your work and discuss how to proceed. Certainly it would be good to utilise the now available DMA API instead of making it hardcoded for the ethernet device. If you have a proper netdev integration all the layers above should come in for "free".

aabadie · 2018-12-17T10:13:20Z

By now it stays as a peripheral but i moved the code from the f4 speicific folder to the stm32_common folder

+1, I think this is the way to go. Feel free to open a new PR so we can review it and test.

Thanks for great work!

crest42 · 2018-12-17T12:55:32Z

Thanks for the fast Feedback. I already put everything in a consumable form. Since i working on top of @lebrush commits anyways credits should be included :) I will open a PR later in the evening and add the open tasks/questions to it.

crest42 · 2018-12-18T17:00:23Z

Opend #10633 and added a few comments. It would be great if someone could guide me a little bit with my questions :)

miri64 · 2018-12-18T17:22:33Z

Let's close this then in favor of #10633

OlegHahm assigned kaspar030 Jul 16, 2016

OlegHahm added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: drivers Area: Device drivers labels Jul 16, 2016

jnohlgard reviewed Jul 18, 2016
View reviewed changes

lebrush force-pushed the stm32f4-eth branch from f39a95e to 0716c0c Compare July 20, 2016 08:34

lebrush force-pushed the stm32f4-eth branch from ff4f424 to 1b62ba2 Compare August 2, 2016 16:44

lebrush force-pushed the stm32f4-eth branch from 1b62ba2 to 6cddda6 Compare August 3, 2016 13:06

This was referenced Aug 3, 2016

cpu/stm32f4: uncrustify periph_cpu #5720

Merged

boards/stm32f4discovery: linux serial name update #5721

Closed

kaspar030 requested changes Sep 24, 2016

View reviewed changes

lebrush force-pushed the stm32f4-eth branch from 64390db to 5b75c3e Compare November 11, 2016 14:48

lebrush force-pushed the stm32f4-eth branch from 5b75c3e to 045c11f Compare December 14, 2016 09:53

lebrush force-pushed the stm32f4-eth branch from 045c11f to 18c76ba Compare December 22, 2016 08:58

lebrush force-pushed the stm32f4-eth branch from 18c76ba to 18fa6ff Compare March 6, 2017 13:42

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng removed this from the Release 2018.01 milestone Jan 12, 2018

miri64 added the State: waiting for other PR label Oct 5, 2018

miri64 removed the State: waiting for other PR label Oct 12, 2018

crest42 mentioned this pull request Dec 18, 2018

drivers: stm32 eth peripheral driver #10633

Merged

miri64 closed this Dec 18, 2018

stm32f4 eth peripheral driver #5643

stm32f4 eth peripheral driver #5643

Conversation

lebrush commented Jul 14, 2016 • edited Loading

jnohlgard commented Jul 16, 2016

kYc0o commented Jul 16, 2016

lebrush commented Jul 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebrush commented Aug 3, 2016

lebrush commented Aug 3, 2016

kaspar030 commented Aug 3, 2016

lebrush commented Aug 3, 2016

lebrush commented Sep 6, 2016

OlegHahm commented Sep 23, 2016

kaspar030 left a comment

Choose a reason for hiding this comment

kaspar030 Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

kaspar030 Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebrush commented Nov 11, 2016

lebrush commented Nov 28, 2016

OlegHahm commented Dec 8, 2016

kYc0o commented Mar 3, 2017

lebrush commented Mar 6, 2017

aabadie commented Mar 6, 2017

Tenderson commented Oct 11, 2017 • edited Loading

miri64 commented Nov 14, 2017

lebrush commented Nov 15, 2017 via email

smlng commented Jan 12, 2018

MrKevinWeiss commented Apr 11, 2018

tcschmidt commented Jun 1, 2018

MrKevinWeiss commented Jun 3, 2018

tcschmidt commented Jun 3, 2018

lebrush commented Jun 3, 2018

smlng commented Jun 4, 2018

MrKevinWeiss commented Jun 4, 2018

tcschmidt commented Jun 11, 2018

smlng commented Jun 11, 2018

tcschmidt commented Jul 4, 2018 • edited Loading

tcschmidt commented Aug 21, 2018

crest42 commented Dec 15, 2018 • edited Loading

crest42 commented Dec 15, 2018 • edited Loading

MrKevinWeiss commented Dec 17, 2018

smlng commented Dec 17, 2018

aabadie commented Dec 17, 2018

crest42 commented Dec 17, 2018

crest42 commented Dec 18, 2018

miri64 commented Dec 18, 2018

lebrush commented Jul 14, 2016 •

edited

Loading

lebrush commented Jul 18, 2016 •

edited

Loading

kaspar030 Sep 24, 2016 •

edited

Loading

kaspar030 Sep 24, 2016 •

edited

Loading

Tenderson commented Oct 11, 2017 •

edited

Loading

tcschmidt commented Jul 4, 2018 •

edited

Loading

crest42 commented Dec 15, 2018 •

edited

Loading

crest42 commented Dec 15, 2018 •

edited

Loading