Page MenuHomeFreeBSD

Add 64-bit support to Cadence CGEM Ethernet driver.
ClosedPublic

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

Details

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
R10 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

share/man/man4/cgem.4
302

But we default to 1 for other devices already?

sys/arm64/conf/GENERIC
156

"Gigabit Ethernet"

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?

Remove some local changes that leaked into last update.

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
47

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

sys/dev/cadence/if_cgem.c
437

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

sys/dev/cadence/if_cgem_hw.h
39

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

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

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
47

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
437

The compiler balks at this in 32-bit mode.

Pull URL to next line in if_cgem_hw.h.

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 inline comments.
share/man/man4/cgem.4
297

There's a missing newline.

298

This should probably be stylized with the Va macro.

Incorporate suggested fixes to man page.

Hi @skibo , is there any remaining work to be done on this patch? It appears to be ready to go, other than one small nitpick. It would be nice to see it committed before stable/13 branches later this month.

share/man/man4/Makefile
906

Generally ${MACHINE_CPUARCH} is preferred since it captures all variants of an architecture. Described further in arch(7).

Hi @skibo , is there any remaining work to be done on this patch? It appears to be ready to go, other than one small nitpick. It would be nice to see it committed before stable/13 branches later this month.

I'd love to see this go into 13.0. I want to sanity check the driver again and then I'll make your suggested change and update, hopefully by tomorrow.

Use MACHINE_CPUARCH in share/man/man4/Makefile.

Use MACHINE_CPUARCH in share/man/man4/Makefile.

Well, that didn't work. Sorry. First time using arc with a git tree.

  • Use MACHINE_CPUARCH in share/man/man4/Makefile.
skibo marked an inline comment as done.Jan 8 2021, 3:52 AM

Makefile update looks good to me! You might want to bump .Dd when you commit.

  • Bump date in man4/cgem.4.

Makefile update looks good to me! You might want to bump .Dd when you commit.

I am not a committer but I bumped the date. Earlier today, I sanity checked the driver on both Zynq board and a Zynq UltraScale so I think it's ready.

Makefile update looks good to me! You might want to bump .Dd when you commit.

I am not a committer but I bumped the date. Earlier today, I sanity checked the driver on both Zynq board and a Zynq UltraScale so I think it's ready.

Ahh, my mistake. Thanks for taking the time to verify it. I can commit it some time this weekend if nobody beats me to it.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 10 2021, 8:54 PM
Closed by commit R10:facdd1cd2045: cgem: add 64-bit support (authored by skibo, committed by mhorne). · Explain Why
This revision was automatically updated to reflect the committed changes.