Page MenuHomeFreeBSD

Introduce Armada 38x/XP network controller support
ClosedPublic

Authored by mw_semihalf.com on May 14 2017, 12:28 AM.
Tags
Referenced Files
Unknown Object (File)
Mon, Mar 18, 4:54 PM
Unknown Object (File)
Wed, Mar 6, 1:59 AM
Unknown Object (File)
Wed, Mar 6, 1:59 AM
Unknown Object (File)
Feb 9 2024, 10:24 AM
Unknown Object (File)
Jan 3 2024, 12:22 AM
Unknown Object (File)
Dec 31 2023, 2:12 AM
Unknown Object (File)
Dec 31 2023, 2:12 AM
Unknown Object (File)
Dec 22 2023, 11:39 PM

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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.

This revision is now accepted and ready to land.Jun 1 2017, 3:58 AM
zbb edited reviewers, added: mw_semihalf.com; removed: zbb.
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

Could these be runtime detected rather than hardcoding one or the other?

sys/dev/neta/if_mvneta.c
73

You shouldn't need this in a device driver.

404

fdt_* functions should be avoided.

553

Could this be runtime detected?

558

This should be at the top of the function.

598

Where is MVNETA_MULTIQUEUE set?

621

Where is this defined?

639–640

Shouldn't you free any allocated resources here? e.g. ifp.

853

Shouldn't the DELAY come after the read? There is always an unneeded call to DELAY this way.

941

if ((foo() & BAR) == 0)

1007

} while ((reg & MVNETA_RQC_EN_MASK) != 0)

1030

And here

1042

And here

1065

What is the meaning of 0x20?

1111

You are clearing bits that must be set?

1164

Is this in the fast path and needs __predict_false? Also, style(9).

1891

Why is this needed? i.e. in what cases should this be enabled/disabled?

3528

Why the tab?

3565

Tab?

sys/dev/neta/if_mvnetareg.h
37

There is inconsistent #define indentation in this file, it should be #define<tab>FOO.

sys/dev/neta/if_mvnetavar.h
33

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
sys/arm/mv/mvwin.h
233–236

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
73

Right, seems like a leftover - will check that.

404

Will switch to ofw_bus_find_compatible().

553

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.

558

Ok.

598

Will create config option.

621

Ditto.

639–640

Good catch, thanks.

853

Right, I'll swap it.

941

Ok.

1007

Ok.

1065

Will replace the magic with macro.

1111

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).

1164

It's not a hotpath, so the branch prediction indeed can be removed.

1891

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.)

3528

Editor issue, will correct it.

sys/dev/neta/if_mvnetareg.h
37

Ok.

sys/dev/neta/if_mvneta.c
558

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.

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 - subversion.
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.