Paths

Table of Contentst

Add genet driver for Raspberry Pi 4B Ethernet
ClosedPublic
Actions

Authored by karels on Apr 16 2020, 12:49 AM.

Details

Reviewers

imp
kevans
cem
manu

Commits

rS360181: Add genet driver for Raspberry Pi 4B Ethernet

Summary

Add driver for Broadcom "GENET" version 5, as found in BCM-2711 on
Raspberry Pi 4B. The driver is derived in part from the bcmgenet.c
driver in NetBSD, along with bcmgenetreg.h.

Test Plan

Tested by several alpha testers

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

karels created this revision.Apr 16 2020, 12:49 AM

Herald added a reviewer: cem. · View Herald TranscriptApr 16 2020, 12:49 AM

Herald added a reviewer: manu. · View Herald Transcript

Herald added subscribers: emaste, andrew. · View Herald Transcript

Harbormaster completed remote builds in B30523: Diff 70630.Apr 16 2020, 12:49 AM

mikael added a subscriber: mikael.Apr 17 2020, 12:10 PM

db added a subscriber: db.Apr 17 2020, 12:49 PM

manu added inline comments.Apr 17 2020, 1:11 PM

sys/arm64/broadcom/genet/if_genet.c
313 ↗	(On Diff #70630)	You need to release the resource before returning here.
350 ↗	(On Diff #70630)	We have mii_fdt_get_contype in sys/dev/mii/mii_fdt.c for this.
sys/conf/files.arm64
182 ↗	(On Diff #70630)	optional SOC_BRCM_BCM2838 fdt genet

I see what you mean now about gen_setup_extres() having been useless for your purposes -- the problem is that our RPi stuff doesn't use the extres clk framework like it should, so these clk/hwreset may be found in the FDT but they aren't locally registered like the framework's expecting. This scared me at first, and it still kinda sucks, but that's baggage that predates your work on this.

would be nice to split the FDT attachment into a separate if_genet_fdt file already, since we know if_genet_acpi will eventually come (here's the acpi table)

skibo added a subscriber: skibo.Apr 17 2020, 8:28 PM

skibo added inline comments.

sys/arm64/broadcom/genet/if_genet.c
1180–1183 ↗	(On Diff #70630)	Weird indentation. Is this leftover debug code?

@greg: I'm not quite sure how the FDT/ACPI split should be done; I think that will have to wait for another time.

sys/arm64/broadcom/genet/if_genet.c
313 ↗	(On Diff #70630)	Which resource?
350 ↗	(On Diff #70630)	Thanks, fixed in next iteration.
1180–1183 ↗	(On Diff #70630)	Yes, thanks Removed in next iteration.

pi added a subscriber: pi.Apr 18 2020, 8:15 AM

yklaxds_gmail.com added a subscriber: yklaxds_gmail.com.Apr 18 2020, 1:44 PM

Updates based on review comments, including teardown after failed attach.

Harbormaster completed remote builds in B30589: Diff 70743.Apr 18 2020, 6:46 PM

In D24436#538192, @greg_unrelenting.technology wrote:

would be nice to split the FDT attachment into a separate if_genet_fdt file already, since we know if_genet_acpi will eventually come (here's the acpi table)

Agreed, even if this could be done later it would be better to split now.

manu added inline comments.Apr 19 2020, 10:39 AM

sys/arm64/broadcom/genet/if_genet.c
332 ↗	(On Diff #70743)	Why not make this function a complete detach ? This would also help for dev/test by only compiling this driver as a module.
375 ↗	(On Diff #70743)	It always returns 0 even if we cannot parse the phy mode, please return 1 here in case of an error.
517 ↗	(On Diff #70743)	This function return void but can fail, if bus_dma* calls fails we will still attach but the interface wouldn't be usable. Better return the error code and handle that in attach().
581 ↗	(On Diff #70743)	bus_dmamap_destroy can fails, I think that there is nothing that we can do if it fails but a warning printf would be nice.
585 ↗	(On Diff #70743)	Same for bus_dma_tag_destroy
1654 ↗	(On Diff #70743)	that doesn't seem right

Is there an existing example of FDT / ACPI split to use as a model?

About doing a full detach: then code is needed to undo the init routine, and there are far more failure modes. I'll look at it though.

In D24436#538847, @karels wrote:

Is there an existing example of FDT / ACPI split to use as a model?

You could have a look at sys/dev/usb/controller/generic_ehci*

In D24436#538847, @karels wrote:

Is there an existing example of FDT / ACPI split to use as a model?

If you want, you could take a look at the following . Thank you :

https://marc.info/?l=openbsd-tech&m=158670721506367&w=2
https://marc.info/?l=openbsd-tech&m=158653301620637&w=2
https://marc.info/?l=openbsd-tech&m=158653487221337&w=2

Address more review comments; fix bug with bpf and txcsum.

Notes:

I started a version with full detach; it is not working correctly yet.

I looked at the FDT/ACPI versions of the generic ehci; is there any
documentation of how inheritance works? I think moving to that framework
would take me a while, so I'm favoring lazy evaluation at the moment.

Harbormaster completed remote builds in B30621: Diff 70795.Apr 20 2020, 12:26 AM

In D24436#538986, @karels wrote:

Address more review comments; fix bug with bpf and txcsum.

Notes:

I started a version with full detach; it is not working correctly yet.

What is the problem exactly ?

I looked at the FDT/ACPI versions of the generic ehci; is there any
documentation of how inheritance works?

I'm not sure ...

I think moving to that framework would take me a while, so I'm favoring lazy evaluation at the moment.

I'm not even sure that we could boot RPI4 with ACPI right now so I guess it's ok to leave this for now.

This looks good for me now, I've not tested the driver as I don't have the hardware but all my comments are now resolved.
Thanks for doing this port.

This revision is now accepted and ready to land.Apr 20 2020, 9:44 PM

In D24436#539201, @manu wrote:

In D24436#538986, @karels wrote:

Address more review comments; fix bug with bpf and txcsum.

Notes:

I started a version with full detach; it is not working correctly yet.

What is the problem exactly ?

I started with code to "stop" on ifconfig down, then re-init on "up". The plan was to use "stop" to undo all the running state, like flushing rings, when doing detach. It was no longer crashing, but not working after re-init.

I looked at the FDT/ACPI versions of the generic ehci; is there any
documentation of how inheritance works?

I'm not sure ...

I think moving to that framework would take me a while, so I'm favoring lazy evaluation at the moment.

I'm not even sure that we could boot RPI4 with ACPI right now so I guess it's ok to leave this for now.

Thanks, that seems best to me too.

I will wait until tomorrow for any other comments, then commit if there is nothing significant/unfixed.

Closed by commit rS360181: Add genet driver for Raspberry Pi 4B Ethernet (authored by karels). · Explain WhyApr 22 2020, 12:42 AM

This revision was automatically updated to reflect the committed changes.

karels added a commit: rS360181: Add genet driver for Raspberry Pi 4B Ethernet.