Page MenuHomeFreeBSD

Update mvneta/e6000sw for new style Distributed Switch Architecture Device Tree Bindings
ClosedPublic

Authored by xistence_0x58.com on Jan 30 2019, 7:05 PM.
Referenced Files
Unknown Object (File)
Wed, Apr 17, 6:42 PM
Unknown Object (File)
Wed, Apr 17, 10:47 AM
Unknown Object (File)
Tue, Apr 16, 5:24 AM
Unknown Object (File)
Tue, Apr 9, 8:11 PM
Unknown Object (File)
Tue, Apr 2, 11:31 AM
Unknown Object (File)
Thu, Mar 28, 8:30 AM
Unknown Object (File)
Feb 23 2024, 12:14 AM
Unknown Object (File)
Jan 31 2024, 9:51 PM

Details

Summary

The new style DSA DTB is documented here for: https://www.kernel.org/doc/Documentation/devicetree/bindings/net/dsa/dsa.txt

This new format is used by the ESPRESSObin dts: https://github.com/freebsd/freebsd/blob/5a7b66a89ee821edb7c05a71a4ba8d30c083f233/sys/gnu/dts/arm64/marvell/armada-3720-espressobin.dts

Which means there is no longer a top-level "marvell,dsa" OFW node, and instead we need to look up the switch directly, and follow the xref if necessary.

This fixes the mvneta_if code that looks to see if it has a switch attached, currently it only looks for a single switch type, I am not sure if there is a better way, but we likely need to walk the whole ofw tree looking for xrefs to ourselves (assuming there is no function out there to do that work for us?)

Second I've fixed the e6000sw to use the same method for finding its switch node, with the change we also get the ability to just look for a child named "ports" and then iterate over the ports themselves individually. I've updated the code that parses the port information as well to use xref to look up the referenced ethernet port to be able to configure the fixed-link (although someone with more experience with device tree bindings/DSA please chime in if that is incorrect).

Last but not least, I've changed the multi_chip selection scheme to not check if the sw_addr is a multiple of 2. This was causing failures to identify the switch correctly on my ESPRESSObin v7 because it would incorrectly go into single chip addressing mode, which meant it was failing to attach to the device correctly.

This is the kernel configuration used for testing:

#
# ESPRESSO -- Generic kernel configuration file for FreeBSD/arm64 for the ESPRESSObin
#
# $FreeBSD: head/sys/arm64/conf/GENERIC-UP 325096 2017-10-29 08:17:03Z eadler $

include         GENERIC
ident           ESPRESSO

device etherswitch
device miiproxy
device iicbus

device mdio
device e6000sw
mvneta0: <NETA controller> mem 0x30000-0x33fff irq 14 on simplebus1
mvneta0: version is 10
mvneta0: Could not acquire MAC address. Using randomized one.
mvneta0: bpf attached
mvneta0: Ethernet address: 74:41:f8:a1:9d:00
mvneta0: This device is attached to a switch
mdio0: <MDIO> on mvneta0
e6000sw0: Found switch_node: 0x147c
e6000sw0: Found switch_node: 0x147c
e6000sw0: <Marvell 88E6341> on mdio0
e6000sw0: multi-chip addressing mode
e6000sw0: CPU port at 0
e6000sw0: fixed port at 0
e6000sw0: PHY at port 1
miibus0: <MII bus> on e6000sw0
e1000phy0: <Marvell 88E1000 Gigabit PHY> PHY 17 on miibus0
e1000phy0: OUI 0x000ac2, model 0x0000, rev. 1
e1000phy0:  none, 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-master, 1000baseT-FDX, 1000baseT-FDX-master, auto
e6000sw0: PHY at port 2
miibus1: <MII bus> on e6000sw0
e1000phy1: <Marvell 88E1000 Gigabit PHY> PHY 18 on miibus1
e1000phy1: OUI 0x000ac2, model 0x0000, rev. 1
e1000phy1:  none, 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-master, 1000baseT-FDX, 1000baseT-FDX-master, auto
e6000sw0: PHY at port 3
miibus2: <MII bus> on e6000sw0
e1000phy2: <Marvell 88E1000 Gigabit PHY> PHY 19 on miibus2
e1000phy2: OUI 0x000ac2, model 0x0000, rev. 1
e1000phy2:  none, 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT, 1000baseT-master, 1000baseT-FDX, 1000baseT-FDX-master, auto
etherswitch0: <Switch controller> on e6000sw0
mdioproxy0: <MII/MDIO proxy, MDIO side> on mdio0
etherswitch0: VLAN mode: PORT
port0:
        flags=1<CPUPORT>
        media: Ethernet 1000baseT <full-duplex>
        status: active
port1:
        flags=0<>
        media: Ethernet autoselect (none)
        status: no carrier
port2:
        flags=0<>
        media: Ethernet autoselect (none)
        status: no carrier
port3:
        flags=0<>
        media: Ethernet autoselect (1000baseT <full-duplex,master>)
        status: active
port4:
        flags=0<>
        media: <unknown type>
        status: no carrier
port5:
        flags=0<>
        media: <unknown type>
        status: no carrier
vlangroup0:
        port: 0
        members 1,2,3
vlangroup1:
        port: 1
        members 0,2,3
vlangroup2:
        port: 2
        members 0,1,3
vlangroup3:
        port: 3
        members 0,1,2

Do note: I have yet to figure out how to get etherswitch hooked up into the rest of the networking in PORT mode (all I can find is docs for dot1q, which is not yet supported by the e6000sw code, and I don't have access to a data sheet), so at this point while the e6000sw is up and running, I have yet to get it to pass packets (and the mvneta0 interface doesn't seem to either).

Help or pointers would be greatly appreciated, etherswitch is woefully under documented.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dteske added inline comments.
sys/dev/etherswitch/e6000sw/e6000sw.c
190 ↗(On Diff #53429)

Removing this blank line breaks style(9). To explain why the blank line was there:

This function declares no variables and as such should have a blank line after the opening curly.

Try to get ahold of mw@ and loos@ to review

Updated to match style(9) thanks to @dteske's comment.

I also have these changes up on Github at: https://github.com/freebsd/freebsd/compare/master...theasylum:fix/e6000sw-fdt if it is easier to review there due to more context. I did re-create the diff as requested by @manu with -U999, so hopefully that helps.

manu requested changes to this revision.Feb 8 2019, 10:42 AM

Added some comments.
Keep up the good work :)

sys/dev/etherswitch/e6000sw/e6000sw.c
226 ↗(On Diff #53662)

I don't see any reference to this property in the binding docs or in the dts.
Does that still exists ?
Or is it again a Marvell specific property that only exists in their u-boot/linux fork ?

294 ↗(On Diff #53662)

I know that it's not part of the review but cpu port don't have a label property.
So we will never set sc->cpuports_mask in this function.
You should remove this block and set cpuports_mask/fixed_mask if the port node have a ethernet property (this seems to be the only way to detect a cpu port).

303 ↗(On Diff #53662)

fixed-link is a property of the port, not the node referenced by the ethernet phandle

313 ↗(On Diff #53662)

speed property is mandatory for fixed-link, you should print a message and return ENXIO if it's not present.

389 ↗(On Diff #53662)

You need to check that ports != 0

This revision now requires changes to proceed.Feb 8 2019, 10:42 AM
sys/dev/etherswitch/e6000sw/e6000sw.c
303 ↗(On Diff #53662)

Meh, bindings says that but the dts do not follow this ...
Maybe we should check both ways then.

Thanks for the patch. After you resubmit, I'd like to try it on the Clearfog board as well.

Best regards,
Marcin

sys/dev/etherswitch/e6000sw/e6000sw.c
226 ↗(On Diff #53662)

Hi Manu,

Indeed this property is not documentet in Linux binding, however it is possible to configure board in a way that we have single chip addressing mode with non-zero mdio addressing. This is a workaround for a problem on the commercial, out-of-tree product (it uses FreeBSD only), therefore I'd prefer to keep it here.

Best regards,
Marcin

303 ↗(On Diff #53662)

In armada-388-clearfog.dtsi it is properly defined as a child node with props inside. However this patch introduces a functional change - it checks the fixed-link only for a port containing 'ethernet' property (i.e. the 'cpu' port). However it is possible to have it also on a 'lan' port, when it's connected to the external PHY (please refer to the clearfog DT as well).

313 ↗(On Diff #53662)

+1

389 ↗(On Diff #53662)

+1

sys/dev/neta/if_mvneta.c
423 ↗(On Diff #53662)

Please do not add XXX comments, anyway mv88e6058 has a typo :)

For finding if current port is referenced as a CPU port of the switch, I think we have 3 options:
a. create const char * array with compatibles and check if any is present in the tree
b. look for mdio nodes by name and scan their children for switch/port
c. scan tree from root "/" for "ports" node and later for "ethernet" property in children

I'd go for c. Below code mostly does this anyway, just extend search to entire DT and the solution will be more generic.

432 ↗(On Diff #53662)

Make sure "ports" is found.

mw requested changes to this revision.Feb 13 2019, 1:04 PM
sys/dev/etherswitch/e6000sw/e6000sw.c
226 ↗(On Diff #53662)

Ok thanks for the clarification, maybe we should add a comment then.

andrew added inline comments.
sys/dev/etherswitch/e6000sw/e6000sw.c
226 ↗(On Diff #53662)

If it's a FreeBSD specific property we should prefix it with freebsd,.

Thanks for the feedback! Will work on this shortly, travel and work have kept me busy.

sys/dev/etherswitch/e6000sw/e6000sw.c
226 ↗(On Diff #53662)

It looks like it was added in a commit by someone at Semihalf:

https://github.com/freebsd/freebsd/commit/879cf7c79fa3a9a665515dc5f832a185f8927295

I haven't touched this part of it (even though I can't find a similar statement in any of the Linux tree), because I didn't want to remove something that was added with a purpose.

294 ↗(On Diff #53662)

I can definitely update this as part of the review, even if it is not something I changed :-)

303 ↗(On Diff #53662)

I will update this code to do the check both ways so that we support either format for the DTS.

389 ↗(On Diff #53662)

If ports is 0, should I just return? A switch with no ports doesn't make much sense.

sys/dev/neta/if_mvneta.c
423 ↗(On Diff #53662)

+1

432 ↗(On Diff #53662)

+1

xistence_0x58.com added inline comments.
sys/dev/etherswitch/e6000sw/e6000sw.c
414 ↗(On Diff #54631)

Style violation, will update indentation.

I have not yet had a chance to verify this all works on my ESPRESSObin.

mw requested changes to this revision.Mar 3 2019, 3:22 PM
mw added inline comments.
sys/arm64/conf/ESPRESSO
14 ↗(On Diff #54632)

Please do not add new files in sys/arm64/conf, modfy GENERIC instead.

sys/dev/etherswitch/e6000sw/e6000sw.c
282 ↗(On Diff #54632)

no need to use {} with single line under 'if'

309 ↗(On Diff #54632)

drop {}

sys/dev/neta/if_mvneta.c
195 ↗(On Diff #54632)

don't add this new line

This revision now requires changes to proceed.Mar 3 2019, 3:22 PM
sys/dev/etherswitch/e6000sw/e6000sw.c
306–307 ↗(On Diff #54632)

Could this be moved into e6000sw_parse_fixed_link (and the same below)?

309 ↗(On Diff #54632)

Or just make it return (e6000sw_parse_fixed_link(...));

xistence_0x58.com marked 6 inline comments as done.

Removed the ESPRESSO kernel config and placed the appropriate devices in the arm64 GENERIC config.

I can confirm that the changes made to if_mvneta.c for doing switch detection work, and it correctly detects the switch. The switch ports and fixed-link speeds are also configured correctly. I missed part of the changes I need to make though, and that is if there is an ethernet device on the port, then we need to set the CPUPORT flag.

Updated the diff to correctly set the cpu mask if the port is connected to an ethernet device.

sys/dev/etherswitch/e6000sw/e6000sw.c
339 ↗(On Diff #54658)

Extra { } for readability on the else.

Thanks for the changes - I had two minor comments to the latest version. Once applied, I will check on the clearfog - if it continues to work, I will approve and commit the change.

sys/dev/neta/if_mvneta.c
446 ↗(On Diff #54659)

nit: please add blank line before return.

829 ↗(On Diff #54659)

Please remove this print for "!has_switch" case. It is most common case and most boards do not have it, so imo it won't look well.

Latest code review comments. Added a newline before a return, and removed the extra boot verbose output.

It looks ok for me now but could you please separate the change for drivers and GENERIC kernel config ?
Thanks.

In D19036#417396, @manu wrote:

It looks ok for me now but could you please separate the change for drivers and GENERIC kernel config ?
Thanks.

I'll do this when commiting, no need to resubmit. I need to only check if clearfog continues to work with this.

@xistence_0x58.com in the meantime, please let know What name/email address you'd like to see as 'Submitted by:' in the commit message.

In D19036#417431, @mw wrote:
In D19036#417396, @manu wrote:

It looks ok for me now but could you please separate the change for drivers and GENERIC kernel config ?
Thanks.

I'll do this when commiting, no need to resubmit. I need to only check if clearfog continues to work with this.

I checked the clear fog DTS, I don't see anything that would break with the current set of changes, so fingers crossed all works well. I don't have a clearfog device to test on though, so if there are changes necessary, feel free to steal the patch and fix up what is missing!

@xistence_0x58.com in the meantime, please let know What name/email address you'd like to see as 'Submitted by:' in the commit message.

Submitted by: Bert JW Regeer <xistence@0x58.com>

Thank you!

In D19036#417431, @mw wrote:
In D19036#417396, @manu wrote:

It looks ok for me now but could you please separate the change for drivers and GENERIC kernel config ?
Thanks.

I'll do this when commiting, no need to resubmit. I need to only check if clearfog continues to work with this.

Thanks Marcin,

@xistence_0x58.com in the meantime, please let know What name/email address you'd like to see as 'Submitted by:' in the commit message.

@mw is there anything else that I need to do for this?

@mw is there anything else that I need to do for this?

Hi,

Sorry for the delay, but I've been on a vacation lately. I was finally able to check, and the clearfog operates properly with your patch, so I'll merge it.

Best regards,
Marcin

This revision is now accepted and ready to land.Mar 22 2019, 11:58 PM
This revision was automatically updated to reflect the committed changes.