cpu/lpc1768: add ethernet driver#22305
Conversation
basilfx
requested review from
MichelRottleuthner,
MrKevinWeiss,
aabadie,
jia200x,
leandrolanzieri and
miri64
as code owners
github-actions
Bot
added
Platform: ARM
basilfx
added
the
Type: new feature
crasbe
added
the
CI: ready for build
basilfx
force-pushed
the
feature/lpc1768_eth
branch
from
1393b11 to
6a742ef
Compare
basilfx
force-pushed
the
feature/lpc1768_eth
branch
from
e7c2cc8 to
257d4a6
Compare
| while (LPC_EMAC->MIND & MIND_BUSY) { | ||
| if (ztimer_now(ZTIMER_MSEC) >= deadline) { |
There was a problem hiding this comment.
If you are spinning for 100ms, better add a ztimer_sleep(ZTIMER_MSEC, 1) in here
There was a problem hiding this comment.
Added (also in the auto-negotiate function).
There was a problem hiding this comment.
@benpicco after testing, I noticed that this won't work. The _mii_wait_idle method can be invoked from the link timer, so doing a sleep in an ISR context, which results in hard fault.
The 100ms is really a safe-guard. The wait should be in the µs range rather than ms range. Having a fail-safe is nice to have though.
Moving the link timer into netdev's ISR is another option I considered, but I don't see a benefit.
I'll add a comment to explain instead.
(this problem is also applicable to the EFM32 ethernet driver.)
There was a problem hiding this comment.
Moving the link timer into netdev's ISR is another option I considered, but I don't see a benefit.
The benefit would be not blocking interrupts for up to 100ms
There was a problem hiding this comment.
Good point. Let me try to see how this works out.
I'll add this as an additional fixup to the intermediate squash (not yet pushed).
There was a problem hiding this comment.
I have moved this to the event handler and it works.
The 1 ms sleep with a max of 100 ms is something I have to reconsider. Typical wait times are way-way less, so if the first wait fails, it will now wait for orders of magnitude longer for the second try. This will block other events to be processed. I haven't measured how often this happens, but given that the link timer runs every 1000 ms, I can imagine this to happen.
I could go back to a simply (bounded) busy-wait without ztimer, or reduce the sleep size to µs range instead.
There was a problem hiding this comment.
Ultimately, I changed it to a µs-bounded loop. The time-out is not 1000 µs, which should (at 2.5 MHz PHY interface connection) be about 20 times the worst case scenario.
I did not add a sleep: other drivers do a unbounded busy-wait, so I see this as an improvement already. The sooner it completes in the event handler, the faster it can continue receiving.
|
Thank you @benpicco for the review. I made a few quick adjustments already, but will leave the remaining/bigger ones for coming week, as I don't have access to hardware right now :-) |
|
Update on the preliminary test results: a ping test with 16-byte increases up to 2048 bytes has been running non-stop for the past two-ish weeks. Apart from a few switch reboots (UniFi stuff), the MCU did not crash/reboot. |
basilfx
force-pushed
the
feature/lpc1768_eth
branch
from
27b0ee4 to
333d248
Compare
|
@benpicco Same as with the EFM32, OK if I do an intermediate squash? |
|
Sure, got ahead and squash |
|
Update on the testing (using 333d248), also including the EFM32, because I do them simultaneously, and my screenshots include both results. The tests I did:
Screenshots of the tests
The LPC1768 driver has troubles with flood pings, even though the other tests were stable. I did notice that the throughput for the LPC1768 was fluctuating, while the EFM32 was almost exactly the same for each run. The flood ping test confirms this: the driver cannot keep up. I found that interesting, given that the LPC1768 is higher clocked (100 MHz vs 72 MHz) and both drivers share a very similar structure. After a bit of help from Claude, I measured the 'RX Finished' interrupt, which indicate that the producer and consumer ring buffer indexes are equal. This is not an issue immediately, but that means that if the code services the interrupts too slow, the next frame will be dropped. I have tried several fixes, such as moving the link timer to the event callback (so it does not block ethernet interrupts), adding/removing the ztimer_sleep instead of busy-looping for MII operations and dropping error frames earlier in receive path. None of that had an impact, or at least it was hard to measure. The thing that has an impact, was reconfiguring the ring buffers. The LPC1768 has peripheral RAM of 32 KiB. Half of it is reserved for USB. Reconfiguring from 4 RX and 4 TX buffers to 8 RX and 2 TX buffers solved the issue. The 4 RX and 4 TX buffer configuration seems to be the use in other projects too, so I consider this to be a sane default. The user can always reconfigure this. This makes me believe that the flood test is really pushing the limits. I don't know if the observed loss is something to worry about. I don't have other boards to test with. I will do some more testing and see if I can improve, but maybe it is good enough (for now) already. Edit: here is another run of last night: LPC1768: EFM32: |
basilfx
force-pushed
the
feature/lpc1768_eth
branch
3 times, most recently
from
d4a35a8 to
3dd395b
Compare
|
@benpicco the last three fixup commits are the latest changes since the intermediate squash. I'm sorry that it is a bit bigger than expected, but looking into the timing/interrupt 'issue' made me find a few other improvements. To summarize:
Same applies to EFM32. These changes are currently being tested. I did some more research on acceptable flood ping drop rate, comparing it to other (cable) connected appliances in my home, and I guess that less than 0.1% is still acceptable for an embedded device. I can probably not improve this any further, and leave it up to the end-user to maybe increase the number of RX buffers if really necessary. Unless there are some blocking findings, I am not planning to overhaul this once more ;-) |
|
Ping + iPerf tests results are in. No packet loss during the pings. Asked Claude to draw it nicely. You can see the sawtooth pattern, which is due to the increasing packet size on every run. LPC1768: 
EFM32: 
In all honesty, there are no differences. But it all still works. The flood ping tests are now running. |
|
And the flood ping tests results (3 hours): LPC1768 (883 packets lost, but less than 0.1%): EFM32 (1 packet lost): The results are still very similar to the previous run. |
|
Go ahead and squash then |
Although the ethernet peripheral isn't a common peripheral, it is best placed under the peripheral configuration in the Kconfig menu.
This commit adds a driver for the ethernet peripheral. It is split into a low-level peripheral driver, and a higher-level netdev driver. Two pseudo-modules are introduced for link state monitoring and auto negotiation. Kconfig support has been added too.
basilfx
force-pushed
the
feature/lpc1768_eth
branch
from
3dd395b to
d7ccbdc
Compare
4 participants
Contribution description
Note
Development of this PR happens in parallel to the EFM32 ethernet driver in #22306. Any feedback on this one is likely applicable to the other one as well. I will try to keep both PRs in sync where it matters.
This PR adds an ethernet driver for the LPC1768. Although this CPU is ancient and not the best-supported one, it is still an active one. It is also very well supported by other projects and I have a board (Seeeduino Arch Pro) that has ethernet support.
I have no former experience with ethernet drivers, so that's why I chose this one. Together with the help of LLMs, I figured this would be an OK challenge, which is a small side-step to get Modbus TCP support testable.
I took the ethernet driver of the SAM0 and the STM32 as a basis, and modelled it closely to that one.
What is provided:
lpc1768_eth_link_up,lpc1768_eth_auto) like the STM32.Testing procedure
The only LPC1768 board with ethernet in RIOT's code base, is the Seeeduino Arch Pro. I don't think many people will own this board, so it is noted that actual testing will a be a bit harder.
At this point, I would rather receive feedback on the general structure and adoption. I will then do my best to collect as much evidence as possible.
Testing has been performed as follows:
tests/drivers/lpc1768_ethWith
lpc1768_eth_autoenabled, the link speed is ~15-20 MBit/sec (measured using lwIP iPerf). Without, it is ~75-100 KBit/sec (classical issue due to link negotation issues).Compile test
Issues/PRs references
None
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are:
cpu/lpc1768. I then refined, reworked and tested it myself. I did ask Claude Code to analyze bugs I encountered during testing. The final PR review was also done with Claude Code, but changes were applied manually. The test application, auto-init adoption and several other parts are simply copy-paste of existing drivers.