Page MenuHomeFreeBSD

Add 64-bit support to Cadence CGEM Ethernet driver.
Needs ReviewPublic

Authored by skibo on Apr 5 2020, 6:17 PM.

Details

Reviewers
manu
philip
Group Reviewers
manpages
Summary

Add 64-bit address support to Cadence CGEM Ethernet driver for use in
other SoCs such as the Zynq UltraScale+ and SiFive Highfive Unleashed.

Test Plan

Testing on Ultra96 board with ethernet mezzanine board. I have tweaked
the Ultra96's DTS so that physical memory is split across low-memory
addresses and the 32G boundary and have verified that high memory
addresses are appearing in transmit and receive descriptors.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32695
Build 30143: arc lint + arc unit

Event Timeline

skibo created this revision.Apr 5 2020, 6:17 PM
Herald added 1 blocking reviewer(s): manu. · View Herald TranscriptApr 5 2020, 6:17 PM
mhorne added a subscriber: mhorne.Apr 5 2020, 7:00 PM
emaste added inline comments.Apr 20 2020, 9:19 PM
share/man/man4/cgem.4
302

But we default to 1 for other devices already?

sys/arm64/conf/GENERIC
169

"Gigabit Ethernet"

skibo updated this revision to Diff 70841.Apr 21 2020, 5:18 PM

Tweak cgem description in config files. Correct man page with regards to which
devices the rxhangwar is enabled.

@kp, @philip, @nick would it be possible to sanity test this on a sifive board?

skibo updated this revision to Diff 70842.Apr 21 2020, 5:26 PM

Remove some local changes that leaked into last update.

skibo marked 2 inline comments as done.Apr 21 2020, 5:31 PM
philip removed a subscriber: philip.

@kp, @philip, @nick would it be possible to sanity test this on a sifive board?

Sure. I'll give it a spin this afternoon or tomorrow morning (UTC+8).

I am a little dubious about the many instances of #if INTPTR_MAX == INT64_MAX compile-time gymnastics.

share/man/man4/cgem.4
48

"among many others"? It's a pretty common IP block.

sys/dev/cadence/if_cgem.c
438

Isn't it clearer to simply always restrict it to 1ULL << 32?

sys/dev/cadence/if_cgem_hw.h
40

Put the URL on a line by itself so it doesn't scroll off the screen quite so far. ;-)

kp added a comment.Apr 22 2020, 3:45 PM

The device tree on my FU540 lists the interface as a 'cdns,macb', which isn't listed in the compat_data any more. It's probably worth keeping an entry for it.

With both HWTYPE_GENERIC_GEM and HWTYPE_SIFIVE_FU540 it work (well, pings do, I've not tested more than that).

skibo added a comment.Apr 22 2020, 6:08 PM

I am a little dubious about the many instances of #if INTPTR_MAX == INT64_MAX compile-time gymnastics.

I'm open to suggestions for something cleaner. In most of those instances it's because the compiler balks when I shift an address 32 bits in armv7.

share/man/man4/cgem.4
48

Is it? The only other place I could find any reference to it was in the Atmel AT91 boards and support for those was pulled out a while ago. Further, the "macb" device is a different enough beast than cgem that the driver in (that other operating system) has separate register definitions and separate routines for initialization and filling and retrieving descriptors. I don't think this driver would work for the macb and that's why I removed "cdns,macb" from the compatibility list.

sys/dev/cadence/if_cgem.c
438

The compiler balks at this in 32-bit mode.

skibo updated this revision to Diff 70883.Apr 22 2020, 6:16 PM

Pull URL to next line in if_cgem_hw.h.

skibo marked an inline comment as done.Apr 22 2020, 6:17 PM
skibo updated this revision to Diff 75217.Fri, Jul 31, 4:12 PM

Simplify support for 64-bit systems.
This diff got ugly because I was trying to support the case of a 32-bit
device in a 64-bit kernel. But, I don't think that would work anyway and
I think it's safe to assume that any 64-bit system (arm64 or riscv64) will
have a 64-bit capable gem core. Also, I have added support for using the
clock infrastructure for changing the transmit reference clock which I need
for my zynqmp implementation.

0mp added a subscriber: 0mp.Fri, Jul 31, 8:39 PM
0mp added inline comments.
share/man/man4/cgem.4
298

There's a missing newline.

299

This should probably be stylized with the Va macro.

skibo updated this revision to Diff 75253.Sat, Aug 1, 5:28 PM

Incorporate suggested fixes to man page.

philip accepted this revision.Sun, Aug 2, 3:27 AM

Looks good to me!