Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Dwmac 5.10a Ethernet Driver #188

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Add Dwmac 5.10a Ethernet Driver #188

wants to merge 18 commits into from

Conversation

Kswin01
Copy link
Contributor

@Kswin01 Kswin01 commented Aug 2, 2024

This PR adds support for dwmac 5.10a ethernet driver, primarily tested on the Pine64 Star64.

These are preliminary performance figures when running the echo_server example. There is still considerable performance improvements to be made.

100000000,99984012,100000009,1472,114,298,596,125.48,264,0,0,0
200000000,199901305,200000256,1472,526,726,1022,103.90,727,0,0,0
300000000,299862749,299999187,1472,593,741,913,77.90,737,0,0,0
400000000,399407536,400000656,1472,660,7716,17033,3880.44,7840,0,0,0
500000000,399606637,499999812,1472,20432,23028,25471,1369.32,23418,0,0,0
600000000,400844578,600000198,1472,17411,20281,21603,1078.68,20957,0,0,0
700000000,397171569,699999003,1472,21293,21660,22086,96.94,21646,0,0,0
800000000,403385132,799999446,1472,14940,16987,19086,1257.58,16937,0,0,0
900000000,396167909,899998410,1472,17099,18860,21215,1406.38,18444,0,0,0
1000000000,383013407,814230967,1472,28458,28874,29394,132.36,28817,0,0,0

Copy link
Contributor

@wom-bat wom-bat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main issues are that we want the driver to be generic enough to use for hte IMX8MP boards as well as the star-V; and we should use hardware checksumming.

@@ -16,6 +16,7 @@
#include <serial_config.h>
#include <ethernet_config.h>

#ifdef CONFIG_ARCH_ARM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#ifdefs in code files need to be factored out. In this case, make separate benchmark-arm.c and benchmark-risc-V.c and either build just one of them in the Makefile, or include just one with


#include "benchmark-" #ARCH ".c"

or similar.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that because there's no seL4 support for benchmarking functionality on RISC-V, we can't really do anything on RISC-V.

drivers/network/starfive/eth_driver.mk Outdated Show resolved Hide resolved
drivers/network/starfive/ethernet.c Outdated Show resolved Hide resolved
drivers/network/starfive/ethernet.c Outdated Show resolved Hide resolved

// For normal transmit descriptors, tdes2 needs to be set to generate an IRQ on transmit
// completion. We also need to provide the length of the buffer data in bits 13:0.
uint32_t tdes2 = DESC_TXCTRL_TXINT | buffer.len;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably only want to interrupt when the next-to last queued Tx buf is processed. There's no point in interrupting for every frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I'll try this change out. However this is the same method that is used in the meson driver, where we are setting the interrupt completion bit on every tx packet.

drivers/network/starfive/ethernet.c Outdated Show resolved Hide resolved
Star64 in the TS machine queue. This address is resident the boards EEPROM, however,
we need I2C to read from this ROM. */
*MAC_REG(GMAC_ADDR_HIGH(0)) = 0x00005b75;
*MAC_REG(GMAC_ADDR_LOW(0)) = 0x0039cf6c;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should make this a #define, as we're using the driver for the IMX8MP as well.

@@ -10,6 +10,7 @@
#include <sddf/util/util.h>
#include <microkit.h>

#ifdef CONFIG_ARCH_ARM
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Find a way to do this without #ifdefs

network/components/virt_rx.c Outdated Show resolved Hide resolved
network/components/virt_tx.c Outdated Show resolved Hide resolved
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
@Kswin01 Kswin01 force-pushed the star64_eth branch 2 times, most recently from 3590857 to f3f5bd5 Compare September 17, 2024 06:39
Kswin01 and others added 8 commits September 24, 2024 16:22
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Rather than having #ifdefs for ARM, we instead have the implementations
handle the architecture specific operations. In RISC-V's case, this is
just a no-op.

I removed `ROUND_UP(buffer.len, 1 << CONFIG_L1_CACHE_LINE_SIZE_BITS)`
because we do the rounding up in `cache_clean_and_invalidate`
anyways. We can't have this line on RISC-V because
`CONFIG_L1_CACHE_LINE_SIZE_BITS` does not exist on non-ARM architectures.

Signed-off-by: Ivan Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop these changes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop these changes.

@Ivan-Velickovic
Copy link
Collaborator

Okay if you just drop the changes of the files I commented on then we just have so sort out the licensing of the register definitions and then we can merge.

Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Signed-off-by: Krishnan Winter <krishnanwinter1@gmail.com>
Manmeet Dhaliwal and others added 2 commits October 10, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants