This patch contains a new driver for the network unit of Marvell
Armada 38x/XP SoCs, called NETA. This support was thoroughly tested
and optimised in terms of stability and performance. Additional
hardware features, like Buffer Management (BM) or Parser and Classifier
(PnC) will be progressively supported as needed.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
For future please upload diffs with context - see https://wiki.freebsd.org/CodeReview (Create a Revision via Web Interface) for info.
It's not a big deal in this case since this consists mainly of new files, but is the desired general practice for Phabricator.
Comment Actions
Added code required for the non-coherent platforms.
Introduced kernel option to enable NETA tweaks when HW I/O coherency is enabled.
Comment Actions
You shouldn't use fdt_ functions here.
| sys/arm/mv/mvwin.h | ||
|---|---|---|
| 233–236 ↗ | (On Diff #29145) | Could these be runtime detected rather than hardcoding one or the other? |
| sys/dev/neta/if_mvneta.c | ||
| 72 ↗ | (On Diff #29145) | You shouldn't need this in a device driver. |
| 403 ↗ | (On Diff #29145) | fdt_* functions should be avoided. |
| 552 ↗ | (On Diff #29145) | Could this be runtime detected? |
| 557 ↗ | (On Diff #29145) | This should be at the top of the function. |
| 597 ↗ | (On Diff #29145) | Where is MVNETA_MULTIQUEUE set? |
| 620 ↗ | (On Diff #29145) | Where is this defined? |
| 638–639 ↗ | (On Diff #29145) | Shouldn't you free any allocated resources here? e.g. ifp. |
| 852 ↗ | (On Diff #29145) | Shouldn't the DELAY come after the read? There is always an unneeded call to DELAY this way. |
| 940 ↗ | (On Diff #29145) | if ((foo() & BAR) == 0) |
| 1006 ↗ | (On Diff #29145) | } while ((reg & MVNETA_RQC_EN_MASK) != 0) |
| 1029 ↗ | (On Diff #29145) | And here |
| 1041 ↗ | (On Diff #29145) | And here |
| 1064 ↗ | (On Diff #29145) | What is the meaning of 0x20? |
| 1110 ↗ | (On Diff #29145) | You are clearing bits that must be set? |
| 1163 ↗ | (On Diff #29145) | Is this in the fast path and needs __predict_false? Also, style(9). |
| 1890 ↗ | (On Diff #29145) | Why is this needed? i.e. in what cases should this be enabled/disabled? |
| 3527 ↗ | (On Diff #29145) | Why the tab? |
| 3564 ↗ | (On Diff #29145) | Tab? |
| sys/dev/neta/if_mvnetareg.h | ||
| 36 ↗ | (On Diff #29145) | There is inconsistent #define indentation in this file, it should be #define<tab>FOO. |
| sys/dev/neta/if_mvnetavar.h | ||
| 32 ↗ | (On Diff #29145) | There is inconsistent #define indentation in this file, it should be #define<tab>FOO. |
| sys/arm/mv/mvwin.h | ||
|---|---|---|
| 233–236 ↗ | (On Diff #29145) | Ok, will introduce separate mv_win_eth_setup routine for mvneta. This way mge and mvneta will be chosen, basing on the compatible string only. |
| sys/dev/neta/if_mvneta.c | ||
| 72 ↗ | (On Diff #29145) | Right, seems like a leftover - will check that. |
| 403 ↗ | (On Diff #29145) | Will switch to ofw_bus_find_compatible(). |
| 552 ↗ | (On Diff #29145) | I'll replace config option with checking ddr_attr field value (in MV_WIN_ETH_BASE() register), configured for mvneta MBUS windows in mv_common.c. |
| 557 ↗ | (On Diff #29145) | Ok. |
| 597 ↗ | (On Diff #29145) | Will create config option. |
| 620 ↗ | (On Diff #29145) | Ditto. |
| 638–639 ↗ | (On Diff #29145) | Good catch, thanks. |
| 852 ↗ | (On Diff #29145) | Right, I'll swap it. |
| 940 ↗ | (On Diff #29145) | Ok. |
| 1006 ↗ | (On Diff #29145) | Ok. |
| 1064 ↗ | (On Diff #29145) | Will replace the magic with macro. |
| 1110 ↗ | (On Diff #29145) | MVNETA_PMACC0_PORTEN field is toggled in mvneta_init_locked/mvneta_stop_locked. Here we are just making sure, there's no unwanted 'enable' state (e.g. leftover from firmware). |
| 1163 ↗ | (On Diff #29145) | It's not a hotpath, so the branch prediction indeed can be removed. |
| 1890 ↗ | (On Diff #29145) | Tests showed, that for some applications multiple tx queues can benefit, but we chose by default single TXQ, due to its slightly better performance. At least until per-CPU operation is implemented in mvneta (binding IRQs to the CPUs, etc.) |
| 3527 ↗ | (On Diff #29145) | Editor issue, will correct it. |
| sys/dev/neta/if_mvnetareg.h | ||
| 36 ↗ | (On Diff #29145) | Ok. |
| sys/dev/neta/if_mvneta.c | ||
|---|---|---|
| 557 ↗ | (On Diff #29145) |
Assuming this is under ifdef we would end up adding two ifdefs instead of one. The second one would contain only one variable declaration.... Is this more reasonable? I don't know, but I have my thoughts. Luckily this will be runtime detected. |
| sys/dev/neta/if_mvneta.c | ||
|---|---|---|
| 557 ↗ | (On Diff #29145) | It's generally how it's done, e.g. https://svnweb.freebsd.org/base/head/sys/kern/vfs_subr.c?revision=319502&view=markup#l2378 |
Comment Actions
- correct style for all #define and other style(9) fixes
- remove magics
- enable dynamic coherency settings detection instead of option config
- remove SoC ifdefs in mv_common and use custom setup/dump routines for neta
- enable setting MVNETA_MULTIQUEUE and MVNETA_KTR as a config option
- optimize DELAY usage in busy wait loops in miibus methods
- add missing resources free on error in init
- remove HWCSUM_IPV6 ifdefs
- replace fdt_find_compatible with ofw_bus_find_compatible