Page MenuHomeFreeBSD

Add genet driver for Raspberry Pi 4B Ethernet
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

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.

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.

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*

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.

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:

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.

This revision was automatically updated to reflect the committed changes.