Skip to contentSkip to conversation

DSA support for Marvell 88e6065 switch

PATCH In review Pavel Machek 9 participants 31 messages Nov 16, 2018 – Jan 29, 2019

Subsystems (5)
Networking Network drivers Device tree bindings Networking DSA +1 more subsystems
lore
Share & tools
lore

Pavel Machek

author
#0 lore raw

Hi!

I'm trying to create support for Marvell 88e6065 switch... and it seems like drivers/net/dsa supports everything, but this model.

Did someone work with this hardware before? Any idea if it would be more suitable to support by existing 88e6060 code, or if 88e6xxx code should serve as a base?

Thanks,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#1 lore raw
Quoted 8 lines · Pavel Machek

Hi!

I'm trying to create support for Marvell 88e6065 switch... and it seems like drivers/net/dsa supports everything, but this model.

Did someone work with this hardware before? Any idea if it would be more suitable to support by existing 88e6060 code, or if 88e6xxx code should serve as a base?

Hi Pavel

The 88e6xxx should be extended to support this. I think you will find a lot of the building blocks are already in the driver. Compare the various implementations of the functions in the mv88e6xxx_ops to what the datasheet says for the registers, and pick those that match.

Andrew

#2 lore raw
Quoted 10 lines · Andrew Lunn
Quoted 8 lines · Pavel Machek

Hi!

I'm trying to create support for Marvell 88e6065 switch... and it seems like drivers/net/dsa supports everything, but this model.

Did someone work with this hardware before? Any idea if it would be more suitable to support by existing 88e6060 code, or if 88e6xxx code should serve as a base?

Quoted 6 lines

Hi Pavel

The 88e6xxx should be extended to support this. I think you will find a lot of the building blocks are already in the driver. Compare the various implementations of the functions in the mv88e6xxx_ops to what the datasheet says for the registers, and pick those that match.

Ok, so I played a bit.

It looks like e6065 has different register layout from those supported by 6xxx, and is quite similar to e6060.

I understand how 88e6xxx code is supposed to work... but I don't understand how e6060 code is supposed to be probed. It does not seem to have device tree support. It seems to be older code, but is way simpler, and seems to be targetted at similar hardware.

Best regards,

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#3 lore raw
Quoted 18 lines · Pavel Machek
Quoted 10 lines · Andrew Lunn
Quoted 8 lines · Pavel Machek

Hi!

I'm trying to create support for Marvell 88e6065 switch... and it seems like drivers/net/dsa supports everything, but this model.

Did someone work with this hardware before? Any idea if it would be more suitable to support by existing 88e6060 code, or if 88e6xxx code should serve as a base?

Quoted 6 lines

Hi Pavel

The 88e6xxx should be extended to support this. I think you will find a lot of the building blocks are already in the driver. Compare the various implementations of the functions in the mv88e6xxx_ops to what the datasheet says for the registers, and pick those that match.

Quoted 4 lines

Ok, so I played a bit.

It looks like e6065 has different register layout from those supported by 6xxx, and is quite similar to e6060.

Hi Pavel

However, if you look in the mv88e6xxx, there are quite a few functions called mv88e6065_foo. Marvell keeps changing the register layout. When writing code, we try to name the functions based on which family of devices introduced those registers. But we don't have the whole history, so we probably have some names wrong.

Quoted 4 lines

I understand how 88e6xxx code is supposed to work... but I don't understand how e6060 code is supposed to be probed. It does not seem to have device tree support. It seems to be older code, but is way simpler, and seems to be targetted at similar hardware.

The e6060 code is really old, pretty much abandoned. To make it usable, you are going to have to make a lot of changes. Turn it into an mdio driver, add device tree, make it use the new dsa2.c API, etc.

I still think you should be looking at the mv88e6xxx driver, but maybe taking bits of code from the 6060 driver and adding it to the mv88e6xxx driver as needed.

Andrew

author
#4 lore raw

HI!

Quoted 19 lines
Quoted 14 lines
Quoted 7 lines
Quoted 6 lines

I'm trying to create support for Marvell 88e6065 switch... and it seems like drivers/net/dsa supports everything, but this model.

Did someone work with this hardware before? Any idea if it would be more suitable to support by existing 88e6060 code, or if 88e6xxx code should serve as a base?

Quoted 6 lines

Hi Pavel

The 88e6xxx should be extended to support this. I think you will find a lot of the building blocks are already in the driver. Compare the various implementations of the functions in the mv88e6xxx_ops to what the datasheet says for the registers, and pick those that match.

Quoted 4 lines

Ok, so I played a bit.

It looks like e6065 has different register layout from those supported by 6xxx, and is quite similar to e6060.

Quoted 7 lines

Hi Pavel

However, if you look in the mv88e6xxx, there are quite a few functions called mv88e6065_foo. Marvell keeps changing the register layout. When writing code, we try to name the functions based on which family of devices introduced those registers. But we don't have the whole history, so we probably have some names wrong.

Let me check... I thought there was only one such function. Ok, I see two such functions:

mv88e6065_phylink_validate
mv88e6065_port_set_speed

I may need to re-check, but it looked to me like even functions and registeres labeled 6xxx are different on 6060 and 6065... which means changes to the code would not be exactly nice and easy.

Quoted 8 lines
Quoted 4 lines

I understand how 88e6xxx code is supposed to work... but I don't understand how e6060 code is supposed to be probed. It does not seem to have device tree support. It seems to be older code, but is way simpler, and seems to be targetted at similar hardware.

The e6060 code is really old, pretty much abandoned. To make it usable, you are going to have to make a lot of changes. Turn it into an mdio driver, add device tree, make it use the new dsa2.c API, etc.

If I wanted it to work, what do I need to do? AFAICT phy autoprobing should just attach it as soon as it is compiled in?

I tried adding nodes in dts trying to make the driver attach, but no luck so far.

Quoted 3 lines

I still think you should be looking at the mv88e6xxx driver, but maybe taking bits of code from the 6060 driver and adding it to the mv88e6xxx driver as needed.

e6060 driver is much simpler, which is somehow nice. Let me look around some more.

Thanks,
Pavel

Signature · 2 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
maintainer
#5 lore raw
Quoted 2 lines

If I wanted it to work, what do I need to do? AFAICT phy autoprobing should just attach it as soon as it is compiled in?

Nope. It is a switch, not a PHY. Switches are never auto-probed because they are not guaranteed to have ID registers.

You need to use the legacy device tree binding. Look in Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated Binding. You can get more examples if you checkout old kernels. Or kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.

Andrew

#6 lore raw
Quoted 10 lines · Andrew Lunn
Quoted 2 lines

If I wanted it to work, what do I need to do? AFAICT phy autoprobing should just attach it as soon as it is compiled in?

Nope. It is a switch, not a PHY. Switches are never auto-probed because they are not guaranteed to have ID registers.

You need to use the legacy device tree binding. Look in Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated Binding. You can get more examples if you checkout old kernels. Or kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.

Thanks; I ported code from mv88e66xx in the meantime, and switch appears to be detected.

But I'm running into problems with tagging code, and I guess I'd like some help understanding.

tag_trailer: allocates new skb, then copies data around.

tag_qca: does dev->stats.tx_packets++, and reuses existing skb.

tag_brcm: reuses existing skb.

Is qca wrong in adjusting the statistics? Why does trailer allocate new skb?

6065 seems to use 2-byte header between "SFD" and "Destination address" in the ethernet frame. That's ... strange place to put header, as addresses are now shifted. I need to put ethernet in promisc mode (by running tcpdump) to get data moving.. and can not figure out what to do in tag_...

Can I model it after tag_qca.c (simplified, because addresses will not need to be memmoved) or is there some special consideration I am missing?

Thanks,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#7 lore raw
Quoted 23 lines · Pavel Machek
Quoted 10 lines
Quoted 2 lines

If I wanted it to work, what do I need to do? AFAICT phy autoprobing should just attach it as soon as it is compiled in?

Nope. It is a switch, not a PHY. Switches are never auto-probed because they are not guaranteed to have ID registers.

You need to use the legacy device tree binding. Look in Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated Binding. You can get more examples if you checkout old kernels. Or kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.

Thanks; I ported code from mv88e66xx in the meantime, and switch appears to be detected.

But I'm running into problems with tagging code, and I guess I'd like some help understanding.

tag_trailer: allocates new skb, then copies data around.

tag_qca: does dev->stats.tx_packets++, and reuses existing skb.

tag_brcm: reuses existing skb.

Quoted 8 lines

Is qca wrong in adjusting the statistics? Why does trailer allocate new skb?

6065 seems to use 2-byte header between "SFD" and "Destination address" in the ethernet frame. That's ... strange place to put header, as addresses are now shifted. I need to put ethernet in promisc mode (by running tcpdump) to get data moving.. and can not figure out what to do in tag_...

Does this switch chip not also support trailer mode?

There's basically four tagging modes for Marvell switch chips: header mode (the one you described), trailer mode (tag_trailer.c), DSA and ethertype DSA. The switch chips I worked on that didn't support (ethertype) DSA tagging did support both header and trailer modes, and I chose to run them in trailer mode for the reasons you describe above, but if your chip doesn't support trailer mode, then yes, you'll have to add support for header mode and put the underlying interface into promiscuous mode and such.

#8 lore raw

Hi!

Quoted 14 lines
Quoted 14 lines
Quoted 10 lines
Quoted 2 lines

If I wanted it to work, what do I need to do? AFAICT phy autoprobing should just attach it as soon as it is compiled in?

Nope. It is a switch, not a PHY. Switches are never auto-probed because they are not guaranteed to have ID registers.

You need to use the legacy device tree binding. Look in Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated Binding. You can get more examples if you checkout old kernels. Or kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.

Thanks; I ported code from mv88e66xx in the meantime, and switch appears to be detected.

Quoted 8 lines
Quoted 8 lines

But I'm running into problems with tagging code, and I guess I'd like some help understanding.

tag_trailer: allocates new skb, then copies data around.

tag_qca: does dev->stats.tx_packets++, and reuses existing skb.

tag_brcm: reuses existing skb.

Any idea why tag trailer allocates new skb, and what is going on with dev->stats.tx_packets++?

Quoted 11 lines
Quoted 8 lines

Is qca wrong in adjusting the statistics? Why does trailer allocate new skb?

6065 seems to use 2-byte header between "SFD" and "Destination address" in the ethernet frame. That's ... strange place to put header, as addresses are now shifted. I need to put ethernet in promisc mode (by running tcpdump) to get data moving.. and can not figure out what to do in tag_...

Does this switch chip not also support trailer mode?

Quoted 8 lines

There's basically four tagging modes for Marvell switch chips: header mode (the one you described), trailer mode (tag_trailer.c), DSA and ethertype DSA. The switch chips I worked on that didn't support (ethertype) DSA tagging did support both header and trailer modes, and I chose to run them in trailer mode for the reasons you describe above, but if your chip doesn't support trailer mode, then yes, you'll have to add support for header mode and put the underlying interface into promiscuous mode and such.

It seems that 6060 supports both header (probably, parts of docs are redacted) and trailer mode... but I'm working with 6065. That does not support trailer mode... or at least word "trailer" does not appear anywhere in the documentation.

What chip were you working with? I may want to take a look on their wording.

6065 indeed has some kind of "egress tagging mode" (with four options), but I have trouble understanding what it really does.

Thanks,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#9 lore raw
Quoted 1 line · Pavel Machek

Hi!

Hello!

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 10 lines
Quoted 2 lines

If I wanted it to work, what do I need to do? AFAICT phy autoprobing should just attach it as soon as it is compiled in?

Nope. It is a switch, not a PHY. Switches are never auto-probed because they are not guaranteed to have ID registers.

You need to use the legacy device tree binding. Look in Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated Binding. You can get more examples if you checkout old kernels. Or kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.

Thanks; I ported code from mv88e66xx in the meantime, and switch appears to be detected.

But I'm running into problems with tagging code, and I guess I'd like some help understanding.

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines

tag_trailer: allocates new skb, then copies data around.

tag_qca: does dev->stats.tx_packets++, and reuses existing skb.

tag_brcm: reuses existing skb.

Any idea why tag trailer allocates new skb,

I wrote this code over 10 years ago, so I don't remember all that well, but I think that it is because you have to do manual checksumming of the packet, as there's no way to pass down the stack that you don't want to checksum all the way down to the end of the data area (and you don't want the tag to be included in the checksum), and so you want to do that before you add the trailer tag, and you'll probably have to reallocate the data area to be able to add the tag, and you probably won't get an exclusive skb here anyway, so you might as well allocate a new one.

Quoted 1 line

and what is going on with dev->stats.tx_packets++?

trailer_xmit would be the hard_start_xmit function for the virtual (slave) network interface, so this would be the right thing to do?

Quoted 20 lines
Quoted 11 lines
Quoted 8 lines

Is qca wrong in adjusting the statistics? Why does trailer allocate new skb?

6065 seems to use 2-byte header between "SFD" and "Destination address" in the ethernet frame. That's ... strange place to put header, as addresses are now shifted. I need to put ethernet in promisc mode (by running tcpdump) to get data moving.. and can not figure out what to do in tag_...

Does this switch chip not also support trailer mode?

Quoted 8 lines

There's basically four tagging modes for Marvell switch chips: header mode (the one you described), trailer mode (tag_trailer.c), DSA and ethertype DSA. The switch chips I worked on that didn't support (ethertype) DSA tagging did support both header and trailer modes, and I chose to run them in trailer mode for the reasons you describe above, but if your chip doesn't support trailer mode, then yes, you'll have to add support for header mode and put the underlying interface into promiscuous mode and such.

Quoted 7 lines

It seems that 6060 supports both header (probably, parts of docs are redacted) and trailer mode... but I'm working with 6065. That does not support trailer mode... or at least word "trailer" does not appear anywhere in the documentation.

What chip were you working with? I may want to take a look on their wording.

I think I added trailer mode just for the 6060, since it doesn't (IIRC) support (ethertype) DSA tagging.

Quoted 2 lines

6065 indeed has some kind of "egress tagging mode" (with four options), but I have trouble understanding what it really does.

What are the options?

#10 lore raw

Hi!

Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
Quoted 8 lines

But I'm running into problems with tagging code, and I guess I'd like some help understanding.

tag_trailer: allocates new skb, then copies data around.

tag_qca: does dev->stats.tx_packets++, and reuses existing skb.

tag_brcm: reuses existing skb.

Any idea why tag trailer allocates new skb,

Quoted 9 lines

I wrote this code over 10 years ago, so I don't remember all that well, but I think that it is because you have to do manual checksumming of the packet, as there's no way to pass down the stack that you don't want to checksum all the way down to the end of the data area (and you don't want the tag to be included in the checksum), and so you want to do that before you add the trailer tag, and you'll probably have to reallocate the data area to be able to add the tag, and you probably won't get an exclusive skb here anyway, so you might as well allocate a new one.

Ok, thanks. Whether tag is checksummed or not is indeed an interesting question.

Quoted 4 lines
Quoted 1 line

and what is going on with dev->stats.tx_packets++?

trailer_xmit would be the hard_start_xmit function for the virtual (slave) network interface, so this would be the right thing to do?

Some tag_*.c do this and some do not, so I'm still confused.

Quoted 4 lines
Quoted 2 lines

6065 indeed has some kind of "egress tagging mode" (with four options), but I have trouble understanding what it really does.

What are the options?

"transmit unmodified", "transmit untagged", "transmit tagged" and "transmit all frames double tagged".

Best regards,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#11 lore raw
Quoted 23 lines · Pavel Machek

Hi!

Quoted 11 lines
Quoted 10 lines
Quoted 8 lines
Quoted 8 lines

But I'm running into problems with tagging code, and I guess I'd like some help understanding.

tag_trailer: allocates new skb, then copies data around.

tag_qca: does dev->stats.tx_packets++, and reuses existing skb.

tag_brcm: reuses existing skb.

Any idea why tag trailer allocates new skb,

Quoted 9 lines

I wrote this code over 10 years ago, so I don't remember all that well, but I think that it is because you have to do manual checksumming of the packet, as there's no way to pass down the stack that you don't want to checksum all the way down to the end of the data area (and you don't want the tag to be included in the checksum), and so you want to do that before you add the trailer tag, and you'll probably have to reallocate the data area to be able to add the tag, and you probably won't get an exclusive skb here anyway, so you might as well allocate a new one.

Quoted 8 lines

Ok, thanks. Whether tag is checksummed or not is indeed an interesting question.

Quoted 4 lines
Quoted 1 line

and what is going on with dev->stats.tx_packets++?

trailer_xmit would be the hard_start_xmit function for the virtual (slave) network interface, so this would be the right thing to do?

Some tag_*.c do this and some do not, so I'm still confused.

This is likely just historical, or we don't have an easy way to determine whether the SKB transmitted succeeded or not because it was reallocated, and therefore the tagging code must make sure statistics are maintained. I suspect it is just historical and that tag module just has not been updated to leverage the statistics accounting done a layer above it within DSA.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

6065 indeed has some kind of "egress tagging mode" (with four options), but I have trouble understanding what it really does.

What are the options?

"transmit unmodified", "transmit untagged", "transmit tagged" and "transmit all frames double tagged".

It seems to me that this applies to VLAN headers, not DSA/trailer headers though not having the data sheet I should be flat out wrong.

Signature · 1 line
Florian
#12 lore raw

Hi!

Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

6065 indeed has some kind of "egress tagging mode" (with four options), but I have trouble understanding what it really does.

What are the options?

"transmit unmodified", "transmit untagged", "transmit tagged" and "transmit all frames double tagged".

It seems to me that this applies to VLAN headers, not DSA/trailer headers though not having the data sheet I should be flat out wrong.

I guess you are right.

And while we probably could use VLAN headers instead of DSA header (in double tagging mode or something), it would likely be complex and error-prone...

Thanks,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#13 lore raw

mv88e6xxx -- DSA support for Marvell 88e6065 switch (and maybe 88e6060?)

In reply to Andrew Lunn

Hi!

Quoted 16 lines
Quoted 16 lines
Quoted 7 lines
Quoted 6 lines

I'm trying to create support for Marvell 88e6065 switch... and it seems like drivers/net/dsa supports everything, but this model.

Did someone work with this hardware before? Any idea if it would be more suitable to support by existing 88e6060 code, or if 88e6xxx code should serve as a base?

Quoted 6 lines

Hi Pavel

The 88e6xxx should be extended to support this. I think you will find a lot of the building blocks are already in the driver. Compare the various implementations of the functions in the mv88e6xxx_ops to what the datasheet says for the registers, and pick those that match.

Ok, so I played a bit.

Quoted 8 lines
Quoted 2 lines

It looks like e6065 has different register layout from those supported by 6xxx, and is quite similar to e6060.

However, if you look in the mv88e6xxx, there are quite a few functions called mv88e6065_foo. Marvell keeps changing the register layout. When writing code, we try to name the functions based on which family of devices introduced those registers. But we don't have the whole history, so we probably have some names wrong.

Ok, so I took a long look at mv88e6xxx... and got it to work.

Good news is that modifications needed are not too heavy. Most are inlined below. (tag_daddr is still needed. I can send that too).

Bad news is that only about half of all the registers are present on 6065 (6060 is similar), and I'm not sure how to do that cleanly. Anything marked "W" is not present or reserved or different in 6065. What would be good markup of registers that are common to all and that are newer-generations-only be? Mark everything not present on 6065 as MV88E6085_*?

Is someone is interested in getting 6060 to work with mv88e6xxx?

Best regards,
Pavel

commit b0a8ed2459b8bc1922441edb482d0c7a4217eb1f

Author: Pavel Machek <[email protected]>
Date:   Mon Jan 28 15:12:30 2019 +0100

6xxx: Add basic support for 6065.

drivers/net/dsa/mv88e6xxx/chip.c +72 -0
index 33e99dfa00a8..0a1fc2432094 100644
@@ -2094,6 +2094,21 @@ static int mv88e6xxx_set_port_mode_edsa(struct mv88e6xxx_chip *chip, int port)
 				       ETH_P_EDSA);
 }
 
+static int mv88e6xxx_set_port_mode_header(struct mv88e6xxx_chip *chip, int port)
+{
+  	u16 reg;
+	int err;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);
+	if (err)
+		return err;
+
+	reg |= MV88E6XXX_PORT_CTL0_HEADER;
+	printk("port %d -- enabling header, val %lx\n", port, (unsigned long) reg);
+
+	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);
+}
+
 static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
 {
 	if (dsa_is_dsa_port(chip->ds, port))

@@ -2109,6 +2124,11 @@ static int mv88e6xxx_setup_port_mode(struct mv88e6xxx_chip *chip, int port)
 	if (chip->info->tag_protocol == DSA_TAG_PROTO_EDSA)
 		return mv88e6xxx_set_port_mode_edsa(chip, port);
 
+	if (chip->info->tag_protocol == DSA_TAG_PROTO_DADDR)
+		return mv88e6xxx_set_port_mode_header(chip, port);
+
+	dev_warn(chip->dev, "set port mode failed");
+
 	return -EINVAL;
 }
 

@@ -2126,6 +2146,7 @@ static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 
 	/* Upstream ports flood frames with unknown unicast or multicast DA */
 	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
+	flood = 1; /* This is strange, but original driver also sets flood everywhere */
 	if (chip->info->ops->port_set_egress_floods)
 		return chip->info->ops->port_set_egress_floods(chip, port,
 							       flood, flood);

@@ -3777,7 +3798,58 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.phylink_validate = mv88e6390x_phylink_validate,
 };
 
+static const struct mv88e6xxx_ops mv88e6065_ops = {
+	/* MV88E6XXX_FAMILY_6095 */ /* Here */
+  //.ieee_pri_map = mv88e6085_g1_ieee_pri_map, /* FIXME */
+  //	.ip_pri_map = mv88e6085_g1_ip_pri_map, /* FIXME */
+  //	.set_switch_mac = mv88e6xxx_g1_set_switch_mac, /* FIXME */
+  	.phy_read = mv88e6185_phy_ppu_read,
+  	.phy_write = mv88e6185_phy_ppu_write,
+  	.port_set_link = mv88e6xxx_port_set_link, /* ok */
+	.port_set_duplex = mv88e6xxx_port_set_duplex, /* ok */
+	.port_set_speed = mv88e6065_port_set_speed, /* ok */
+	.port_set_frame_mode = mv88e6065_port_set_frame_mode, // -- definitely not compatible on 6065; neccessary for correct operation? */
+	.port_set_egress_floods = mv88e6352_port_set_egress_floods, /* ok */
+	//.port_set_upstream_port = mv88e6095_port_set_upstream_port, /* FIXME */
+	.port_link_state = mv88e6352_port_link_state, /* FIXME -- this one is wrong, 6065 uses dufferebt bits */
+	//.port_get_cmode = mv88e6185_port_get_cmode, /* FIXME -- wrong bits */
+	//.stats_snapshot = mv88e6xxx_g1_stats_snapshot, /* FIXME */
+	//.stats_set_histogram = mv88e6095_g1_stats_set_histogram, /* FIXME */
+	//.stats_get_sset_count = mv88e6095_stats_get_sset_count, /* FIXME */
+	//.stats_get_strings = mv88e6095_stats_get_strings, /* FIXME */
+	//.stats_get_stats = mv88e6095_stats_get_stats, /* FIXME */
+	//.mgmt_rsvd2cpu = mv88e6185_g2_mgmt_rsvd2cpu, /* FIXME */
+	//.ppu_enable = mv88e6185_g1_ppu_enable, /* FIXME */
+	//.ppu_disable = mv88e6185_g1_ppu_disable, /* FIXME */
+	.reset = NULL, /* No reset bit in global1: register 4, but we have MV88E6065_G1_RESET */
+	.vtu_getnext = mv88e6185_g1_vtu_getnext, /* ok? */
+	.vtu_loadpurge = mv88e6185_g1_vtu_loadpurge, /* FIXME */
+	.phylink_validate = mv88e6065_phylink_validate,
+};
+
 static const struct mv88e6xxx_info mv88e6xxx_table[] = {
+  	[MV88E6065] = {
+		.prod_num = 0x650,
+		.family = MV88E6XXX_FAMILY_6065,
+		.name = "Marvell 88E6065 not so hacked :-)",
+		.num_databases = 16,
+		.num_ports = 6,
+		.num_internal_phys = 6,
+		.max_vid = 4095,
+		.port_base_addr = 0x18,
+		.phy_base_addr = 0x10,
+		.global1_addr = 0x1f,
+		.global2_addr = 0,
+		.age_time_coeff = 15000,
+		.g1_irqs = 8,
+		.g2_irqs = 0,
+		.atu_move_port_mask = 0xf,
+		.pvt = false,
+		.multi_chip = true,
+		.tag_protocol = DSA_TAG_PROTO_DADDR,
+		.ops = &mv88e6065_ops,
+	},
+
 	[MV88E6085] = {
 		.prod_num = MV88E6XXX_PORT_SWITCH_ID_PROD_6085,
 		.family = MV88E6XXX_FAMILY_6097,
drivers/net/dsa/mv88e6xxx/chip.h +2 -0
index fc82085d4296..7d6c746d4b66 100644
@@ -57,10 +57,12 @@ enum mv88e6xxx_frame_mode {
 	MV88E6XXX_FRAME_MODE_DSA,
 	MV88E6XXX_FRAME_MODE_PROVIDER,
 	MV88E6XXX_FRAME_MODE_ETHERTYPE,
+	MV88E6XXX_FRAME_MODE_HEADER,
 };
 
 /* List of supported models */
 enum mv88e6xxx_model {
+  	MV88E6065,
 	MV88E6085,
 	MV88E6095,
 	MV88E6097,
drivers/net/dsa/mv88e6xxx/port.c +25 -0
index 3f43c231f40b..cfa16b5fd41a 100644
@@ -631,6 +631,31 @@ int mv88e6xxx_port_set_egress_mode(struct mv88e6xxx_chip *chip, int port,
 	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);
 }
 
+int mv88e6065_port_set_frame_mode(struct mv88e6xxx_chip *chip, int port,
+				  enum mv88e6xxx_frame_mode mode)
+{
+  	int err;
+	u16 reg;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, &reg);
+	if (err)
+		return err;
+
+	reg &= ~MV88E6XXX_PORT_CTL0_FRAME_MODE_MASK;
+
+	switch (mode) {
+	case MV88E6XXX_FRAME_MODE_NORMAL:
+	  /* FIXME ... ?  why is it needed */
+		reg |= MV88E6XXX_PORT_CTL0_FRAME_MODE_NORMAL;
+		break;
+	default:
+	  printk("Bad frame mode\n");
+		return -EINVAL;
+	}
+
+	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL0, reg);
+}
+
 int mv88e6085_port_set_frame_mode(struct mv88e6xxx_chip *chip, int port,
 				  enum mv88e6xxx_frame_mode mode)
 {
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

commit 58a8b67092027331b83311e5621e12870fef0845

Author: Pavel Machek <[email protected]>
Date:   Mon Jan 28 14:59:36 2019 +0100

6xxx: headers-only: anotate same and different registers on 6065.

drivers/net/dsa/mv88e6xxx/global1.h +50 -50

219 lines not on this page · raw message

drivers/net/dsa/mv88e6xxx/global2.h +25 -25

167 lines not on this page · raw message

drivers/net/dsa/mv88e6xxx/port.h +102 -97

314 lines not on this page · raw message

maintainer
#14 lore raw

Re: mv88e6xxx -- DSA support for Marvell 88e6065 switch (and maybe 88e6060?)

Quoted 1 line

Is someone is interested in getting 6060 to work with mv88e6xxx?

Vivien is.

I also have a set of patches which modernize the driver. That might be a step towards merging it in.

drivers/net/dsa/mv88e6xxx/chip.c static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port) · line 2149

Quoted 5 lines
drivers/net/dsa/mv88e6xxx/chip.c +1 -0
@@ -2126,6 +2146,7 @@ static int mv88e6xxx_setup_egress_floods(struct mv88e6xxx_chip *chip, int port)
 
 	/* Upstream ports flood frames with unknown unicast or multicast DA */
 	flood = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port);
+	flood = 1; /* This is strange, but original driver also sets flood everywhere */

This might be because the driver did not support hardware offload. What happens if you don't have this?

drivers/net/dsa/mv88e6xxx/chip.c static const struct mv88e6xxx_ops mv88e6390x_ops = { · line 3805

Quoted 5 lines
drivers/net/dsa/mv88e6xxx/chip.c +5 -0
+static const struct mv88e6xxx_ops mv88e6065_ops = {
+	/* MV88E6XXX_FAMILY_6095 */ /* Here */
+  //.ieee_pri_map = mv88e6085_g1_ieee_pri_map, /* FIXME */
+  //	.ip_pri_map = mv88e6085_g1_ip_pri_map, /* FIXME */
+  //	.set_switch_mac = mv88e6xxx_g1_set_switch_mac, /* FIXME */

What you should do is see if the hardware supports these functions. If it does not, simply leave it out of mv88e6065_ops. If it does have the functionality, but needs a new implementation, add a mv88e6065_ version. In theory, nearly everything is optional. So you can start simple and then add features.

Andrew

#15 lore raw

Re: mv88e6xxx -- DSA support for Marvell 88e6065 switch (and maybe 88e6060?)

In reply to Pavel Machek

Hi Pavel,

Quoted 5 lines · Pavel Machek

Ok, so I took a long look at mv88e6xxx... and got it to work.

Good news is that modifications needed are not too heavy. Most are inlined below. (tag_daddr is still needed. I can send that too).

Quoted 8 lines

Bad news is that only about half of all the registers are present on 6065 (6060 is similar), and I'm not sure how to do that cleanly. Anything marked "W" is not present or reserved or different in 6065. What would be good markup of registers that are common to all and that are newer-generations-only be? Mark everything not present on 6065 as MV88E6085_*?

Is someone is interested in getting 6060 to work with mv88e6xxx?

I am currently trying to request boards with an 88E6060 so that we can finally get rid of the legacy code used by the mv88e6060 driver.

I think the 6065 has similar registers layout than the 6060. If so, it would be simpler for you to add the 6065 support into the mv88e6060 driver.

Merging mv88e6060 into mv88e6xxx is the next step I have on my plate after removing legacy.

Are you using the legacy binding with mv88e6xxx or the new ones? If you're using the new ones, Andrew may have a patch to convert the mv88e6060 driver to use the new bindings, that you could apply and test first. Then you can add support for 88E6065 in the mv88e6060 driver more easily.

Thanks,

Vivien

#16 lore raw

Well supported DSA switches, support for Marvell 88e6065 switch

In reply to Pavel Machek

Hi!

Quoted 2 lines

I'm trying to create support for Marvell 88e6065 switch... and it seems like drivers/net/dsa supports everything, but this model.

I have something that works, but is far from suitable for mainline. Few parts are good, those will be marked PATCH, rest will be RFD. I'm not sure how much work I'll be able to put into them in future, so here they go.

Is there well supported switch, preferably with public documentation? Does not have to be Marvell... Three ports could be enough, and 100MBit is enough for our use. We need RSTP, and are using two VLANs...

AFAICT 88e6176 should be quite safe choice (as it is used by Turris Omnia), but it has more ports than we need, and supports gigabit -- that means expensive. It also lacks public documentation AFAICT :-(.

Patches are relavite to v4.19.

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#17 lore raw

PATCH 1/1 mv88e6060: Warn about errors

Errors communicating with the chip are not expected, warn about them.

Signed-off-by Pavel Machek <[email protected]>
drivers/net/dsa/mv88e6060.c +6 -2
index 65f10fec25b3..f43104f48dbb 100644
@@ -30,8 +30,10 @@ static int reg_read(struct dsa_switch *ds, int addr, int reg)
 		int __ret;					\
 								\
 		__ret = reg_read(ds, addr, reg);		\
-		if (__ret < 0)					\
+		if (__ret < 0) {				\
+			printk("mv88e6060: error reading %x/%x\n", addr, reg);	\
 			return __ret;				\
+		}						\
 		__ret;						\
 	})
 

@@ -48,8 +50,10 @@ static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
 		int __ret;					\
 								\
 		__ret = reg_write(ds, addr, reg, val);		\
-		if (__ret < 0)					\
+		if (__ret < 0)  {                               \
+			printk("mv88e6060: error writing %x/%x\n", addr, reg);	\
 			return __ret;				\
+		}						\
 	})
 
 static const char *mv88e6060_get_name(struct mii_bus *bus, int sw_addr)
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#18 lore raw

Re: [PATCH] mv88e6060: Warn about errors

Plain "printk" are never appropriate.

Please explicitly use pr_warn() or similar. If there is a device context available, either a generic device or a netdev, use one of the dev_*() or netdev_*() variants.

#19 lore raw

Re: [PATCH] mv88e6060: Warn about errors

Quoted 6 lines · David Miller

Plain "printk" are never appropriate.

Please explicitly use pr_warn() or similar. If there is a device context available, either a generic device or a netdev, use one of the dev_*() or netdev_*() variants.

Can do, I guess.... is there's agreeement that such error is worth some kind of output to the logs?

Best regards,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#20 lore raw

[RFD] mv88e6060: Allow the driver to be probed from device tree

In reply to Pavel Machek

mv88e6060: Allow the driver to be probed from device tree

This is NOT ready for merge. Hardcoding an address is a bad idea, and obviously it would be good to allow module removal, too.

Signed-off-by Pavel Machek <[email protected]> (not ready)
drivers/net/dsa/mv88e6060.c +107 -11
index f43104f48dbb..9e3901f18518 100644
@@ -77,26 +77,40 @@ static const char *mv88e6060_get_name(struct mii_bus *bus, int sw_addr)
 static enum dsa_tag_protocol mv88e6060_get_tag_protocol(struct dsa_switch *ds,
 							int port)
 {
+  //return DSA_TAG_PROTO_QCA;
+  //return DSA_TAG_PROTO_TRAILER;
 	return DSA_TAG_PROTO_TRAILER;
 }
 
+static struct mv88e6060_priv *
+alloc_priv(struct device *dsa_dev, struct mii_bus *bus, int sw_addr)
+{
+  	struct mv88e6060_priv *priv;
+
+	priv = devm_kzalloc(dsa_dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return NULL;
+
+	priv->bus = bus;
+	priv->sw_addr = sw_addr;
+	return priv;
+}
+
 static const char *mv88e6060_drv_probe(struct device *dsa_dev,
 				       struct device *host_dev, int sw_addr,
 				       void **_priv)
 {
 	struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
-	struct mv88e6060_priv *priv;
 	const char *name;
 
 	name = mv88e6060_get_name(bus, sw_addr);
-	if (name) {
-		priv = devm_kzalloc(dsa_dev, sizeof(*priv), GFP_KERNEL);
-		if (!priv)
-			return NULL;
-		*_priv = priv;
-		priv->bus = bus;
-		priv->sw_addr = sw_addr;
-	}
+	
+	if (!name)
+		return NULL;
+
+	*_priv = alloc_priv(dsa_dev, bus, sw_addr);
+	if (!*_priv)
+		  return NULL;
 
 	return name;
 }

@@ -132,9 +146,12 @@ static int mv88e6060_switch_reset(struct dsa_switch *ds)
 
 		usleep_range(1000, 2000);
 	}
-	if (time_after(jiffies, timeout))
+	if (time_after(jiffies, timeout)) {
+		printk("e6060: reset timed out!\n");
 		return -ETIMEDOUT;
+	}
 
+	printk("e6060: reset ok\n");	
 	return 0;
 }
 

@@ -284,15 +301,94 @@ static struct dsa_switch_driver mv88e6060_switch_drv = {
 	.ops		= &mv88e6060_switch_ops,
 };
 
+static int mv88e6060_probe(struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+	struct device_node *np = dev->of_node;
+	const struct mv88e6060_info *compat_info;
+	struct mv88e6060_priv *chip;
+	u32 eeprom_len;
+	int err;
+
+	int addr = 0x10 /* mdiodev->addr */ ;
+
+	chip = alloc_priv(dev, mdiodev->bus, addr);
+	if (!chip)
+		return -ENOMEM;
+
+	{
+		char *name = mv88e6060_get_name(mdiodev->bus, addr);
+		printk("e6060: got name %s @ %lx %lx\n", name, mdiodev->bus, addr);
+	}
+	{
+	  	struct dsa_switch *ds;
+
+		ds = dsa_switch_alloc(dev, 6);
+		if (!ds)
+			return -ENOMEM;
+
+		ds->priv = chip;
+		ds->dev = dev;
+		ds->ops = &mv88e6060_switch_ops;
+		ds->ageing_time_min = 15000;
+		ds->ageing_time_max = 15000 * U8_MAX;
+
+		dev_set_drvdata(dev, ds);
+
+		return dsa_register_switch(ds);
+	}
+
+	printk("e6060: probe ok\n");
+	return 0;
+}
+
+static void mv88e6060_remove(struct mdio_device *mdiodev)
+{
+	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
+	struct mv88e6060_chip *chip = ds->priv;
+
+	printk("e6060: remove.\n");
+	BUG();
+#if 0
+	mv88e6060_phy_destroy(chip);
+	mv88e6060_unregister_switch(chip);
+	mv88e6060_mdio_unregister(chip);
+#endif
+}
+
+static const struct of_device_id mv88e6060_of_match[] = {
+	{
+		.compatible = "marvell,mv88e6060",
+	},
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, mv88e6060_of_match);
+
+static struct mdio_driver mv88e6060_driver = {
+	.probe	= mv88e6060_probe,
+	.remove = mv88e6060_remove,
+	.mdiodrv.driver = {
+		.name = "mv88e6060",
+		.of_match_table = mv88e6060_of_match,
+	},
+};
+
 static int __init mv88e6060_init(void)
 {
+	int r;
+	printk("e6060: init\n");
 	register_switch_driver(&mv88e6060_switch_drv);
-	return 0;
+	r = mdio_driver_register(&mv88e6060_driver);
+	printk("e6060: init %d\n", r);
+	return r;
 }
+
 module_init(mv88e6060_init);
 
 static void __exit mv88e6060_cleanup(void)
 {
+  	mdio_driver_unregister(&mv88e6060_driver);
 	unregister_switch_driver(&mv88e6060_switch_drv);
 }
 module_exit(mv88e6060_cleanup);
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
build bot
#21 lore raw

Re: [RFD] mv88e6060: Allow the driver to be probed from device tree

I love your patch! Perhaps something to improve:

Hi Pavel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on v4.20-rc5 next-20181206]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Pavel-Machek/mv88e6060-Allow-the-driver-to-be-probed-from-device-tree/20181207-013430
config: i386-randconfig-x007-201848 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All warnings (new ones prefixed by >>):

drivers/net/dsa/mv88e6060.c: In function 'mv88e6060_probe':

>> drivers/net/dsa/mv88e6060.c:316:16: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
      char *name = mv88e6060_get_name(mdiodev->bus, addr);
                   ^~~~~~~~~~~~~~~~~~
>> drivers/net/dsa/mv88e6060.c:317:34: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'struct mii_bus *' [-Wformat=]
      printk("e6060: got name %s @ %lx %lx\n", name, mdiodev->bus, addr);
                                   ~~^               ~~~~~~~~~~~~
>> drivers/net/dsa/mv88e6060.c:317:38: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'int' [-Wformat=]
      printk("e6060: got name %s @ %lx %lx\n", name, mdiodev->bus, addr);
                                       ~~^
                                       %x
   drivers/net/dsa/mv88e6060.c:307:6: warning: unused variable 'err' [-Wunused-variable]
     int err;
         ^~~
   drivers/net/dsa/mv88e6060.c:306:6: warning: unused variable 'eeprom_len' [-Wunused-variable]
     u32 eeprom_len;
         ^~~~~~~~~~
   drivers/net/dsa/mv88e6060.c:304:31: warning: unused variable 'compat_info' [-Wunused-variable]
     const struct mv88e6060_info *compat_info;
                                  ^~~~~~~~~~~
   drivers/net/dsa/mv88e6060.c:303:22: warning: unused variable 'np' [-Wunused-variable]
     struct device_node *np = dev->of_node;
                         ^~
   drivers/net/dsa/mv88e6060.c: In function 'mv88e6060_remove':
   drivers/net/dsa/mv88e6060.c:344:25: warning: unused variable 'chip' [-Wunused-variable]
     struct mv88e6060_chip *chip = ds->priv;
                            ^~~~

Cyclomatic Complexity 1 include/linux/device.h:devm_kzalloc
Cyclomatic Complexity 1 include/linux/device.h:dev_get_drvdata Cyclomatic Complexity 1 include/linux/device.h:dev_set_drvdata Cyclomatic Complexity 1 include/linux/etherdevice.h:eth_random_addr Cyclomatic Complexity 1 include/net/dsa.h:dsa_to_port
Cyclomatic Complexity 1 include/net/dsa.h:dsa_is_cpu_port
Cyclomatic Complexity 1 include/net/dsa.h:dsa_is_user_port
Cyclomatic Complexity 3 include/net/dsa.h:dsa_user_ports
Cyclomatic Complexity 1 drivers/net/dsa/mv88e6060.c:mv88e6060_get_tag_protocol Cyclomatic Complexity 2 drivers/net/dsa/mv88e6060.c:alloc_priv Cyclomatic Complexity 2 drivers/net/dsa/mv88e6060.c:mv88e6060_port_to_phy_addr Cyclomatic Complexity 1 drivers/net/dsa/mv88e6060.c:mv88e6060_init Cyclomatic Complexity 0 drivers/net/dsa/mv88e6060.c:mv88e6060_remove Cyclomatic Complexity 1 drivers/net/dsa/mv88e6060.c:reg_read
Cyclomatic Complexity 2 drivers/net/dsa/mv88e6060.c:mv88e6060_phy_read Cyclomatic Complexity 1 drivers/net/dsa/mv88e6060.c:reg_write
Cyclomatic Complexity 2 drivers/net/dsa/mv88e6060.c:mv88e6060_phy_write Cyclomatic Complexity 3 drivers/net/dsa/mv88e6060.c:mv88e6060_setup_global Cyclomatic Complexity 4 drivers/net/dsa/mv88e6060.c:mv88e6060_setup_addr Cyclomatic Complexity 6 drivers/net/dsa/mv88e6060.c:mv88e6060_setup_port Cyclomatic Complexity 13 drivers/net/dsa/mv88e6060.c:mv88e6060_switch_reset Cyclomatic Complexity 6 drivers/net/dsa/mv88e6060.c:mv88e6060_setup Cyclomatic Complexity 5 drivers/net/dsa/mv88e6060.c:mv88e6060_get_name Cyclomatic Complexity 3 drivers/net/dsa/mv88e6060.c:mv88e6060_probe Cyclomatic Complexity 3 drivers/net/dsa/mv88e6060.c:mv88e6060_drv_probe Cyclomatic Complexity 1 drivers/net/dsa/mv88e6060.c:mv88e6060_cleanup Cyclomatic Complexity 1 drivers/net/dsa/mv88e6060.c:_GLOBAL__sub_I_00100_0_mv88e6060.c Cyclomatic Complexity 1 drivers/net/dsa/mv88e6060.c:_GLOBAL__sub_D_00100_1_mv88e6060.c

vim +/const +316 drivers/net/dsa/mv88e6060.c

299
300 static int mv88e6060_probe(struct mdio_device *mdiodev)
301 {
302 struct device *dev = &mdiodev->dev;
303 struct device_node *np = dev->of_node;
304 const struct mv88e6060_info *compat_info;
305 struct mv88e6060_priv *chip;
306 u32 eeprom_len;
307 int err;
308
309 int addr = 0x10 /* mdiodev->addr */ ;
310
311 chip = alloc_priv(dev, mdiodev->bus, addr);
312 if (!chip)
313 return -ENOMEM;
314
315 {

 > 316			char *name = mv88e6060_get_name(mdiodev->bus, addr);
 > 317			printk("e6060: got name %s @ %lx %lx\n", name, mdiodev->bus, addr);

318 }
319 {
320 struct dsa_switch *ds;
321
322 ds = dsa_switch_alloc(dev, 6);
323 if (!ds)
324 return -ENOMEM;
325
326 ds->priv = chip;
327 ds->dev = dev;
328 ds->ops = &mv88e6060_switch_ops;
329 ds->ageing_time_min = 15000;
330 ds->ageing_time_max = 15000 * U8_MAX;
331
332 dev_set_drvdata(dev, ds);
333
334 return dsa_register_switch(ds);
335 }
336
337 printk("e6060: probe ok\n");
338 return 0;
339 }
340

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

build bot
#22 lore raw

Re: [RFD] mv88e6060: Allow the driver to be probed from device tree

In reply to Pavel Machek
I love your patch! Perhaps something to improve:

Hi Pavel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on v4.20-rc6 next-20181214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Pavel-Machek/mv88e6060-Allow-the-driver-to-be-probed-from-device-tree/20181207-013430
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All warnings (new ones prefixed by >>):

>> drivers/net/dsa/mv88e6060.c:316:48: warning: incorrect type in initializer (different modifiers)
   drivers/net/dsa/mv88e6060.c:316:48:    expected char *name
   drivers/net/dsa/mv88e6060.c:316:48:    got char const *
   drivers/net/dsa/mv88e6060.c: In function 'mv88e6060_probe':
   drivers/net/dsa/mv88e6060.c:316:16: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
      char *name = mv88e6060_get_name(mdiodev->bus, addr);
                   ^~~~~~~~~~~~~~~~~~
   drivers/net/dsa/mv88e6060.c:317:34: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'struct mii_bus *' [-Wformat=]
      printk("e6060: got name %s @ %lx %lx\n", name, mdiodev->bus, addr);
                                   ~~^               ~~~~~~~~~~~~
   drivers/net/dsa/mv88e6060.c:317:38: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'int' [-Wformat=]
      printk("e6060: got name %s @ %lx %lx\n", name, mdiodev->bus, addr);
                                       ~~^
                                       %x
   drivers/net/dsa/mv88e6060.c:307:6: warning: unused variable 'err' [-Wunused-variable]
     int err;
         ^~~
   drivers/net/dsa/mv88e6060.c:306:6: warning: unused variable 'eeprom_len' [-Wunused-variable]
     u32 eeprom_len;
         ^~~~~~~~~~
   drivers/net/dsa/mv88e6060.c:304:31: warning: unused variable 'compat_info' [-Wunused-variable]
     const struct mv88e6060_info *compat_info;
                                  ^~~~~~~~~~~
   drivers/net/dsa/mv88e6060.c:303:22: warning: unused variable 'np' [-Wunused-variable]
     struct device_node *np = dev->of_node;
                         ^~
   drivers/net/dsa/mv88e6060.c: In function 'mv88e6060_remove':
   drivers/net/dsa/mv88e6060.c:344:25: warning: unused variable 'chip' [-Wunused-variable]
     struct mv88e6060_chip *chip = ds->priv;
                            ^~~~

vim +316 drivers/net/dsa/mv88e6060.c

299
300 static int mv88e6060_probe(struct mdio_device *mdiodev)
301 {
302 struct device *dev = &mdiodev->dev;
303 struct device_node *np = dev->of_node;
304 const struct mv88e6060_info *compat_info;
305 struct mv88e6060_priv *chip;
306 u32 eeprom_len;
307 int err;
308
309 int addr = 0x10 /* mdiodev->addr */ ;
310
311 chip = alloc_priv(dev, mdiodev->bus, addr);
312 if (!chip)
313 return -ENOMEM;
314
315 {

 > 316			char *name = mv88e6060_get_name(mdiodev->bus, addr);

317 printk("e6060: got name %s @ %lx %lx\n", name, mdiodev->bus, addr);
318 }
319 {
320 struct dsa_switch *ds;
321
322 ds = dsa_switch_alloc(dev, 6);
323 if (!ds)
324 return -ENOMEM;
325
326 ds->priv = chip;
327 ds->dev = dev;
328 ds->ops = &mv88e6060_switch_ops;
329 ds->ageing_time_min = 15000;
330 ds->ageing_time_max = 15000 * U8_MAX;
331
332 dev_set_drvdata(dev, ds);
333
334 return dsa_register_switch(ds);
335 }
336
337 printk("e6060: probe ok\n");
338 return 0;
339 }
340

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#23 lore raw

mv88e6060: Turn e6060 driver into e6065 driver

In reply to Pavel Machek

This is NOT ready for merge, as you lose support for 6060. Probably not what you want.

Signed-off-by Pavel Machek <[email protected]> (not ready)
drivers/net/dsa/mv88e6060.c +21 -7
index 9e3901f18518..9c9b4792bbad 100644
@@ -61,6 +61,7 @@ static const char *mv88e6060_get_name(struct mii_bus *bus, int sw_addr)
 	int ret;
 
 	ret = mdiobus_read(bus, sw_addr + REG_PORT(0), PORT_SWITCH_ID);
+	printk("e6060: get name: %lx\n", (long) ret);
 	if (ret >= 0) {
 		if (ret == PORT_SWITCH_ID_6060)
 			return "Marvell 88E6060 (A0)";

@@ -69,6 +70,8 @@ static const char *mv88e6060_get_name(struct mii_bus *bus, int sw_addr)
 			return "Marvell 88E6060 (B0)";
 		if ((ret & PORT_SWITCH_ID_6060_MASK) == PORT_SWITCH_ID_6060)
 			return "Marvell 88E6060";
+		if ((ret & PORT_SWITCH_ID_6060_MASK) == 0x0650)
+			return "Marvell 88E6065";
 	}
 
 	return NULL;

@@ -79,7 +82,7 @@ static enum dsa_tag_protocol mv88e6060_get_tag_protocol(struct dsa_switch *ds,
 {
   //return DSA_TAG_PROTO_QCA;
   //return DSA_TAG_PROTO_TRAILER;
-	return DSA_TAG_PROTO_TRAILER;
+	return DSA_TAG_PROTO_DADDR;
 }
 
 static struct mv88e6060_priv *

@@ -128,6 +131,7 @@ static int mv88e6060_switch_reset(struct dsa_switch *ds)
 			  ret & ~PORT_CONTROL_STATE_MASK);
 	}
 
+
 	/* Wait for transmit queues to drain. */
 	usleep_range(2000, 4000);
 

@@ -174,6 +178,11 @@ static int mv88e6060_setup_global(struct dsa_switch *ds)
 	return 0;
 }
 
+static int mv88e6060_setup_port_hacks(struct dsa_switch *ds, int p)
+{
+}
+
+
 static int mv88e6060_setup_port(struct dsa_switch *ds, int p)
 {
 	int addr = REG_PORT(p);

@@ -184,12 +193,17 @@ static int mv88e6060_setup_port(struct dsa_switch *ds, int p)
 	 * port, enable Ingress and Egress Trailer tagging mode.
 	 */
 	REG_WRITE(addr, PORT_CONTROL,
-		  dsa_is_cpu_port(ds, p) ?
-			PORT_CONTROL_TRAILER |
-			PORT_CONTROL_INGRESS_MODE |
-			PORT_CONTROL_STATE_FORWARDING :
-			PORT_CONTROL_STATE_FORWARDING);
-
+		  PORT_CONTROL_INITIAL_PRI_6065 |
+		  PORT_CONTROL_FLOOD_ALL_6065 |
+		  PORT_CONTROL_STATE_FORWARDING |
+		  (dsa_is_cpu_port(ds, p) ?
+		   PORT_CONTROL_HEADER |
+		   PORT_CONTROL_TRAILER_6065
+		   : 0));
+
+	if (dsa_is_cpu_port(ds, p))
+		mv88e6060_setup_port_hacks(ds, p);
+	
 	/* Port based VLAN map: give each port its own address
 	 * database, allow the CPU port to talk to each of the 'real'
 	 * ports, and allow each of the 'real' ports to only talk to
drivers/net/dsa/mv88e6060.h +13 -10
index 10249bd16292..13f7e8a2107a 100644
@@ -32,27 +32,30 @@
 #define PORT_SWITCH_ID_6060_MASK	0xfff0
 #define PORT_SWITCH_ID_6060_R1	0x0601
 #define PORT_SWITCH_ID_6060_R2	0x0602
-#define PORT_CONTROL		0x04
+#define PORT_CONTROL		0x04 /* Matches */
 #define PORT_CONTROL_FORCE_FLOW_CTRL	BIT(15)
-#define PORT_CONTROL_TRAILER	BIT(14)
-#define PORT_CONTROL_HEADER	BIT(11)
-#define PORT_CONTROL_INGRESS_MODE	BIT(8)
+#define PORT_CONTROL_TRAILER_6060	BIT(14)
+#define PORT_CONTROL_TRAILER_6065	(BIT(12) /* |  BIT(13) */)
+#define PORT_CONTROL_HEADER	BIT(11) /* Controls ingress/egress on 6065, 2 byte headers? */
+#define PORT_CONTROL_INGRESS_MODE_6060	BIT(8)
 #define PORT_CONTROL_VLAN_TUNNEL	BIT(7)
-#define PORT_CONTROL_STATE_MASK	0x03
+#define PORT_CONTROL_INITIAL_PRI_6065   BIT(4)
+#define PORT_CONTROL_FLOOD_ALL_6065     (BIT(2) | BIT(3))
+#define PORT_CONTROL_STATE_MASK	0x03 /* Matches */
 #define PORT_CONTROL_STATE_DISABLED	0x00
 #define PORT_CONTROL_STATE_BLOCKING	0x01
 #define PORT_CONTROL_STATE_LEARNING	0x02
 #define PORT_CONTROL_STATE_FORWARDING	0x03
-#define PORT_VLAN_MAP		0x06
+#define PORT_VLAN_MAP		0x06 /* Matches */
 #define PORT_VLAN_MAP_DBNUM_SHIFT	12
 #define PORT_VLAN_MAP_TABLE_MASK	0x1f
-#define PORT_ASSOC_VECTOR	0x0b
+#define PORT_ASSOC_VECTOR	0x0b /* Matches */
 #define PORT_ASSOC_VECTOR_MONITOR	BIT(15)
 #define PORT_ASSOC_VECTOR_PAV_MASK	0x1f
-#define PORT_RX_CNTR		0x10
-#define PORT_TX_CNTR		0x11
+#define PORT_RX_CNTR		0x10 /* Matches */
+#define PORT_TX_CNTR		0x11 /* Matches */
 
-#define REG_GLOBAL		0x0f
+#define REG_GLOBAL		0x0f 
 #define GLOBAL_STATUS		0x00
 #define GLOBAL_STATUS_SW_MODE_MASK	(0x3 << 12)
 #define GLOBAL_STATUS_SW_MODE_0	(0x0 << 12)
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#24 lore raw

Re: mv88e6060: Turn e6060 driver into e6065 driver

From: Pavel Machek <[email protected]>
Date: Thu, 6 Dec 2018 14:03:45 +0100

drivers/net/dsa/mv88e6060.c static enum dsa_tag_protocol mv88e6060_get_tag_protocol(struct dsa_switch *ds, · line 84

Quoted 4 lines
drivers/net/dsa/mv88e6060.c +0 -0
@@ -79,7 +82,7 @@ static enum dsa_tag_protocol mv88e6060_get_tag_protocol(struct dsa_switch *ds,
 {
   //return DSA_TAG_PROTO_QCA;
   //return DSA_TAG_PROTO_TRAILER;

These C++ style comments are not in any of my tree(s).

Your patch submission really needs to shape up if you want your patches to be considered seriously.

Thank you.

#25 lore raw

Re: mv88e6060: Turn e6060 driver into e6065 driver

Quoted 10 lines · David Miller

From: Pavel Machek <[email protected]>
Date: Thu, 6 Dec 2018 14:03:45 +0100

drivers/net/dsa/mv88e6060.c static enum dsa_tag_protocol mv88e6060_get_tag_protocol(struct dsa_switch *ds, · line 84

Quoted 4 lines
drivers/net/dsa/mv88e6060.c +0 -0
@@ -79,7 +82,7 @@ static enum dsa_tag_protocol mv88e6060_get_tag_protocol(struct dsa_switch *ds,
 {
   //return DSA_TAG_PROTO_QCA;
   //return DSA_TAG_PROTO_TRAILER;

These C++ style comments are not in any of my tree(s).

Quoted 2 lines

Your patch submission really needs to shape up if you want your patches to be considered seriously.

This one should have been "RFD". It has way more serious problems then this, as changelog tried to explain.

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#26 lore raw

PATCH 1/1 dsa device tree bindings: fix typo and wrong example

In reply to Pavel Machek

Fix typo and fix compatible value that is not actually permitted by the description in the example.

Signed-off-by Pavel Machek <[email protected]>
Documentation/devicetree/bindings/net/dsa/marvell.txt +2 -2
index feb007af13cb..abf1be036ac5 100644
@@ -41,7 +41,7 @@ Optional properties:
 - mdio			: Container of PHY and devices on the switches MDIO
 			  bus.
 - mdio?		: Container of PHYs and devices on the external MDIO
-			  bus. The node must contains a compatible string of
+			  bus. The node must contain a compatible string of
 			  "marvell,mv88e6xxx-mdio-external"
 
 Example:

@@ -80,7 +80,7 @@ Example:
 		#interrupt-cells = <2>;
 
 		switch0: switch@0 {
-			compatible = "marvell,mv88e6390";
+			compatible = "marvell,mv88e6190";
 			reg = <0>;
 			reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#27 lore raw

Re: [PATCH] dsa device tree bindings: fix typo and wrong example

Quoted 3 lines · Pavel Machek

Fix typo and fix compatible value that is not actually permitted by the description in the example.

Ahoj Pavle, I think the subject should be more like:

	"dt-bindings: net: dsa: ..."

BR, Michal

Quoted 27 lines
Signed-off-by Pavel Machek <[email protected]>
Documentation/devicetree/bindings/net/dsa/marvell.txt +2 -2
index feb007af13cb..abf1be036ac5 100644
@@ -41,7 +41,7 @@ Optional properties:
  - mdio			: Container of PHY and devices on the switches MDIO
  			  bus.
  - mdio?		: Container of PHYs and devices on the external MDIO
-			  bus. The node must contains a compatible string of
+			  bus. The node must contain a compatible string of
  			  "marvell,mv88e6xxx-mdio-external"
 
  Example:

@@ -80,7 +80,7 @@ Example:
  		#interrupt-cells = <2>;
 
  		switch0: switch@0 {
-			compatible = "marvell,mv88e6390";
+			compatible = "marvell,mv88e6190";
  			reg = <0>;
  			reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
#28 lore raw

Re: [PATCH] dsa device tree bindings: fix typo and wrong example

Quoted 8 lines · Vokáč Michal
Quoted 3 lines · Pavel Machek

Fix typo and fix compatible value that is not actually permitted by the description in the example.

Ahoj Pavle, I think the subject should be more like:

	"dt-bindings: net: dsa: ..."

Ahoj!

Well, retrying with different subject only makes sense if someone is interested. Rob, Andrew -- can you take the patch?

Do you want me to edit the subject or can you just edit the subject while applying?

Thanks,
Pavel

Quoted 29 lines

BR, Michal

Quoted 27 lines
Signed-off-by Pavel Machek <[email protected]>
Documentation/devicetree/bindings/net/dsa/marvell.txt +2 -2
index feb007af13cb..abf1be036ac5 100644
@@ -41,7 +41,7 @@ Optional properties:
  - mdio			: Container of PHY and devices on the switches MDIO
  			  bus.
  - mdio?		: Container of PHYs and devices on the external MDIO
-			  bus. The node must contains a compatible string of
+			  bus. The node must contain a compatible string of
  			  "marvell,mv88e6xxx-mdio-external"
 
  Example:

@@ -80,7 +80,7 @@ Example:
  		#interrupt-cells = <2>;
 
  		switch0: switch@0 {
-			compatible = "marvell,mv88e6390";
+			compatible = "marvell,mv88e6190";
  			reg = <0>;
  			reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#29 lore raw

Re: [PATCH] dsa device tree bindings: fix typo and wrong example

Quoted 10 lines · Pavel Machek
Quoted 8 lines · Vokáč Michal
Quoted 3 lines · Pavel Machek

Fix typo and fix compatible value that is not actually permitted by the description in the example.

Ahoj Pavle, I think the subject should be more like:

	"dt-bindings: net: dsa: ..."
Quoted 7 lines

Ahoj!

Well, retrying with different subject only makes sense if someone is interested. Rob, Andrew -- can you take the patch?

Do you want me to edit the subject or can you just edit the subject while applying?

The next steps would likely be for you to resubmit this patch with a re-subjected patch and I am fairly sure David can pick it up right away since this is not a functional change.

Quoted 34 lines

Thanks,
Pavel

Quoted 29 lines

BR, Michal

Quoted 27 lines
Signed-off-by Pavel Machek <[email protected]>
Documentation/devicetree/bindings/net/dsa/marvell.txt +2 -2
index feb007af13cb..abf1be036ac5 100644
@@ -41,7 +41,7 @@ Optional properties:
  - mdio			: Container of PHY and devices on the switches MDIO
  			  bus.
  - mdio?		: Container of PHYs and devices on the external MDIO
-			  bus. The node must contains a compatible string of
+			  bus. The node must contain a compatible string of
  			  "marvell,mv88e6xxx-mdio-external"
 
  Example:

@@ -80,7 +80,7 @@ Example:
  		#interrupt-cells = <2>;
 
  		switch0: switch@0 {
-			compatible = "marvell,mv88e6390";
+			compatible = "marvell,mv88e6190";
  			reg = <0>;
  			reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;
Signature · 1 line
Florian
#30 lore raw

RFC 1/1 tag_daddr: add tagging scheme used by Marvel 88e6065 switch

In reply to Pavel Machek

Tagging scheme used by 88e6065 is "interesting" as it moves around ethernet addreses and forces us to use PROMISC mode on the ethernets.

I'm not sure how to call it, so I selected tag_daddr ("tag where destination address should be").

Signed-off-by Pavel Machek <[email protected]>
include/net/dsa.h +1 -0
index 461e8a7661b7..187da100d468 100644
@@ -40,6 +40,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_MTK,
 	DSA_TAG_PROTO_QCA,
 	DSA_TAG_PROTO_TRAILER,
+	DSA_TAG_PROTO_DADDR,
 	DSA_TAG_LAST,		/* MUST BE LAST */
 };
net/dsa/Kconfig +3 -0
index 4183e4ba27a5..1c177b6c5127 100644
@@ -32,6 +32,9 @@ config NET_DSA_TAG_BRCM
 config NET_DSA_TAG_BRCM_PREPEND
 	bool
 
+config NET_DSA_TAG_DADDR
+	bool "Support for DADDR tagging, found on Marvell 88e6065" 
+
 config NET_DSA_TAG_DSA
 	bool
net/dsa/Makefile +1 -0
index 9e4d3536f977..4466c1b860cc 100644
@@ -7,6 +7,7 @@ dsa_core-$(CONFIG_NET_DSA_LEGACY) += legacy.o
 # tagging formats
 dsa_core-$(CONFIG_NET_DSA_TAG_BRCM) += tag_brcm.o
 dsa_core-$(CONFIG_NET_DSA_TAG_BRCM_PREPEND) += tag_brcm.o
+dsa_core-$(CONFIG_NET_DSA_TAG_DADDR) += tag_daddr.o
 dsa_core-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
 dsa_core-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.o
 dsa_core-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
net/dsa/dsa.c +3 -0
index 9f3209ff7ffd..97d289b0ef9d 100644
@@ -46,6 +46,9 @@ const struct dsa_device_ops *dsa_device_ops[DSA_TAG_LAST] = {
 #ifdef CONFIG_NET_DSA_TAG_BRCM_PREPEND
 	[DSA_TAG_PROTO_BRCM_PREPEND] = &brcm_prepend_netdev_ops,
 #endif
+#ifdef CONFIG_NET_DSA_TAG_DADDR
+	[DSA_TAG_PROTO_DADDR] = &daddr_netdev_ops,
+#endif
 #ifdef CONFIG_NET_DSA_TAG_DSA
 	[DSA_TAG_PROTO_DSA] = &dsa_netdev_ops,
 #endif
net/dsa/dsa_priv.h +3 -0
index 3964c6f7a7c0..0530b125ef72 100644
@@ -199,6 +199,9 @@ void dsa_switch_unregister_notifier(struct dsa_switch *ds);
 extern const struct dsa_device_ops brcm_netdev_ops;
 extern const struct dsa_device_ops brcm_prepend_netdev_ops;
 
+/* tag_daddr.c */
+extern const struct dsa_device_ops daddr_netdev_ops;
+
 /* tag_dsa.c */
 extern const struct dsa_device_ops dsa_netdev_ops;
net/dsa/tag_daddr.c new file +100 -0

100 lines not on this page · raw message

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html