Add StarFive JH7110's Ethernet (eqos & mcommphy modifications, a new board specific file)
Needs ReviewPublic
Actions

Authored by jsihv_gmx.com on Sat, Jun 15, 9:29 PM.

Details

Reviewers

manu
sos
ganbold

Group Reviewers

riscv

Summary

This commit includes what is needed to get JH7110's Ethernet working

CSR clock's frequency should have been given on datasheets but it isn't there. The value is based on its frequency on vendor OS's debug data.

Changing the network speed is tested by switching between 1000M & 100M cables

Witness warns about lock order reversal when setting CSR clock's frequency. These locks are located in softclock code and clk.c. I think it's a real LOR and not a false positive but it only appears during the boot and when connecting an Ethernet cable so maybe we can live with it? I doubt it may be laborous to come up with an alternative way to set the clock frequency.

iperf3 test gives 719 Mbits/sec transfer rate (and has some retrys) while with vendor's OS it's closer to 1000 Mbits/sec. On basis of what I've heard this can be attributed to shortcomings of the eqos driver.

Port switching issue remains unsolved. Ethernet device does not change automatically when switching a cable to another port (as seen in netstat -r and arp -a) and even by using manual commands I've only managed to switch from eqos0 to eqos1 (and not the other way around). No matter how much I've put time in investigating this issue, I've not yet found the ultimate cause. Hopefully the problem can be fixed in a near future.

(When using kernel debug configurations it may take a minute or two that the connection appears when booting without a cable and connecting it afterwards. Without debug options it's much faster)

the style check has been applied to if_eqos.c

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jsihv_gmx.com created this revision.Sat, Jun 15, 9:29 PM

Herald added a subscriber: imp. · View Herald TranscriptSat, Jun 15, 9:29 PM

jsihv_gmx.com requested review of this revision.Sat, Jun 15, 9:29 PM

Thanks for this, could you split the style(9) changes in a other review please ?
Also please re-upload the diff with context (git format-patch -U999 will do the job).

This revision now requires changes to proceed.Sun, Jun 16, 6:44 AM

manu added inline comments.Sun, Jun 16, 6:49 AM

sys/riscv/starfive/starfive_if_eqos.c
94	Part of this function should probably be made generic with part of eqos_fdt_init, some clocks are the same (and have the same fdt property name).

manu added a reviewer: sos.Sun, Jun 16, 6:49 AM

manu added a reviewer: ganbold.

Now with the full context and without style corrections for if_eqos.c

emaste added a subscriber: emaste.Tue, Jun 18, 3:34 PM

I investigated the LOR (lock order reversal) issue further by temporarily removing eqos locks entirely. Witness still keeps on talking about eqos lock, so it possibly rightly notices a connection between the eqos driver and callout API linked to its operations. Debugger's lock commands were pretty uninformative but by using printfs I became pretty sure the lock really is located in softclock (callout API) code (while the other lock involved in LOR is in the generic clock code, clk.c).

sys/riscv/starfive/starfive_if_eqos.c
94	Property name checking could written as a generic code though Rockchip & JH7110 have little if any overlap there. There's now one shared clock name. In any case, there clearly is grounds to separate a generic FDT code at some point if we get more support for boards using Eqos driver but it probably would be an overkill at this moment.