Page MenuHomeFreeBSD

Introduce Armada 38x/XP network controller support
ClosedPublic

Authored by mw_semihalf.com on May 14 2017, 12:28 AM.

Details

Summary

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste added a subscriber: emaste.May 16 2017, 9:04 PM

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.

loos accepted this revision.Jun 1 2017, 3:58 AM
This revision is now accepted and ready to land.Jun 1 2017, 3:58 AM
zbb commandeered this revision.Jun 2 2017, 3:04 PM
zbb edited reviewers, added: mw_semihalf.com; removed: zbb.
zbb updated this revision to Diff 29145.Jun 2 2017, 3:07 PM
zbb edited edge metadata.

Added code required for the non-coherent platforms.
Introduced kernel option to enable NETA tweaks when HW I/O coherency is enabled.

This revision now requires review to proceed.Jun 2 2017, 3:07 PM
andrew requested changes to this revision.Jun 2 2017, 6:03 PM

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.

This revision now requires changes to proceed.Jun 2 2017, 6:03 PM
mw_semihalf.com added inline comments.Jun 3 2017, 7:54 AM
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.

zbb added inline comments.Jun 3 2017, 11:30 AM
sys/dev/neta/if_mvneta.c
557 ↗(On Diff #29145)

This should be at the top of the function.

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.

andrew added inline comments.Jun 5 2017, 8:24 AM
sys/dev/neta/if_mvneta.c
557 ↗(On Diff #29145)
mw_semihalf.com commandeered this revision.Jun 8 2017, 3:02 AM
mw_semihalf.com edited reviewers, added: zbb; removed: mw_semihalf.com.
mw_semihalf.com set the repository for this revision to rS FreeBSD src repository.
mw_semihalf.com edited edge metadata.
  • 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
This revision was automatically updated to reflect the committed changes.