[RFC net-next PATCH 0/3] net: dsa: mv88e6xxx: Add RMU support

[RFC net-next PATCH 0/3] net: dsa: mv88e6xxx: Add RMU support

[Mattias Forsblad]

The Marvell SOHO switches have the ability to receive and transmit                                                                                                    
Remote Management Frames (Frame2Reg) to the CPU through the                                                                                                           
switch attached network interface.                                                                                                                                    
These frames is handled by the Remote Management Unit (RMU) in                                                                                                        
the switch.                                                                                                                                                           
These frames can contain different payloads:                                                                                                                          
single switch register read and writes, daisy chained switch                                                                                                          
register read and writes, RMON/MIB dump/dump clear and ATU dump.                                                                                                      
The dump functions are very costly over MDIO but it's                                                                                                                 
only a couple of network packets via the RMU. Handling these                                                                                                          
operations via RMU instead of MDIO also relieves access                                                                                                               
contention on the MDIO bus.                                                                                                                                           
                                                                                                                                                                      
This request for comment series implements RMU layer 2 and                                                                                                            
layer 3 handling and also collecting RMON counters                                                                                                                    
through the RMU.                                                                                                                                                      
                                                                                                                                                                      
Next step could be to implement single read and writes but we've                                                                                                      
found that the gain to transfer this to RMU is neglible.                                                                                                              
                                                                                                                                                                      
Regards,                                                                                                                                                              
Mattias Forsblad                                                                                                                                                      

Mattias Forsblad (3):
  dsa: Add ability to handle RMU frames.
  mv88e6xxx: Implement remote management support (RMU)
  mv88e6xxx: rmon: Use RMU to collect rmon data.

 drivers/net/dsa/mv88e6xxx/Makefile  |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c    |  60 +++++--
 drivers/net/dsa/mv88e6xxx/chip.h    |  20 +++
 drivers/net/dsa/mv88e6xxx/global1.c |  84 +++++++++
 drivers/net/dsa/mv88e6xxx/global1.h |   3 +
 drivers/net/dsa/mv88e6xxx/rmu.c     | 256 ++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/rmu.h     |  18 ++
 include/net/dsa.h                   |   7 +
 include/uapi/linux/if_ether.h       |   1 +
 net/dsa/tag_dsa.c                   | 102 ++++++++++-
 10 files changed, 536 insertions(+), 16 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.h

-- 
2.25.1

[RFC net-next PATCH 1/3] dsa: Add ability to handle RMU frames.

[Mattias Forsblad]

Support handling of layer 2 part for RMU frames which is
handled in-band with other DSA traffic.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 include/net/dsa.h             |   7 +++
 include/uapi/linux/if_ether.h |   1 +
 net/dsa/tag_dsa.c             | 102 +++++++++++++++++++++++++++++++++-
 3 files changed, 107 insertions(+), 3 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index b902b31bebce..80955a9d5fd6 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -92,6 +92,7 @@ struct dsa_switch;
 struct dsa_device_ops {
 	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
 	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
+	int (*inband_xmit)(struct sk_buff *skb, struct net_device *dev, int seq_no);
 	void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
 			     int *offset);
 	int (*connect)(struct dsa_switch *ds);
@@ -1189,6 +1190,12 @@ struct dsa_switch_ops {
 	void	(*master_state_change)(struct dsa_switch *ds,
 				       const struct net_device *master,
 				       bool operational);
+
+	/*
+	 * RMU operations
+	 */
+	int (*inband_receive)(struct dsa_switch *ds, struct sk_buff *skb,
+			int seq_no);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index d370165bc621..9de1bdc7cccc 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -158,6 +158,7 @@
 #define ETH_P_MCTP	0x00FA		/* Management component transport
 					 * protocol packets
 					 */
+#define ETH_P_RMU_DSA   0x00FB          /* RMU DSA protocol */
 
 /*
  *	This is an Ethernet frame header.
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index e4b6e3f2a3db..58b02109e4cf 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -123,6 +123,83 @@ enum dsa_code {
 	DSA_CODE_RESERVED_7    = 7
 };
 
+#define DSA_RMU_RESV1   0x3e
+#define DSA_RMU         1
+#define DSA_RMU_PRIO    6
+#define DSA_RMU_RESV2   0xf
+
+static int dsa_inband_xmit_ll(struct sk_buff *skb, struct net_device *dev,
+			      const u8 *header, int header_len, int seq_no)
+{
+	static const u8 dest_addr[ETH_ALEN] = { 0x01, 0x50, 0x43, 0x00, 0x00, 0x00 };
+	struct dsa_port *dp;
+	struct ethhdr *eth;
+	u8 *data;
+
+	if (!dev)
+		return -ENODEV;
+
+	dp = dsa_slave_to_port(dev);
+	if (!dp)
+		return -ENODEV;
+
+	/* Create RMU L2 header */
+	data = skb_push(skb, 6);
+	data[0] = (DSA_CMD_FROM_CPU << 6) | dp->ds->index;
+	data[1] = DSA_RMU_RESV1 << 2 | DSA_RMU << 1;
+	data[2] = DSA_RMU_PRIO << 5 | DSA_RMU_RESV2;
+	data[3] = seq_no;
+	data[4] = 0;
+	data[5] = 0;
+
+	/* Add header if any */
+	if (header) {
+		data = skb_push(skb, header_len);
+		memcpy(data, header, header_len);
+	}
+
+	/* Create MAC header */
+	eth = (struct ethhdr *)skb_push(skb, 2 * ETH_ALEN);
+	memcpy(eth->h_source, dev->dev_addr, ETH_ALEN);
+	memcpy(eth->h_dest, dest_addr, ETH_ALEN);
+
+	skb->protocol = htons(ETH_P_RMU_DSA);
+
+	dev_queue_xmit(skb);
+
+	return 0;
+}
+
+static int dsa_inband_rcv_ll(struct sk_buff *skb, struct net_device *dev)
+{
+	int source_device, source_port;
+	struct dsa_switch *ds;
+	u8 *dsa_header;
+	int rcv_seqno;
+	int ret = 0;
+
+	if (!dev || !dev->dsa_ptr)
+		return 0;
+
+	ds = dev->dsa_ptr->ds;
+
+	dsa_header = skb->data - 2;
+
+	source_device = dsa_header[0] & 0x1f;
+	source_port = (dsa_header[1] >> 3) & 0x1f;
+	ds = dsa_switch_find(ds->dst->index, source_device);
+
+	/* Get rcv seqno */
+	rcv_seqno = dsa_header[3];
+
+	skb_pull(skb, DSA_HLEN);
+
+	if (ds->ops && ds->ops->inband_receive(ds, skb, rcv_seqno))
+		netdev_err(dev, "DSA inband: error decoding packet");
+
+	return ret;
+}
+
 static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 				   u8 extra)
 {
@@ -218,9 +295,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 
 		switch (code) {
 		case DSA_CODE_FRAME2REG:
-			/* Remote management is not implemented yet,
-			 * drop.
-			 */
+			dsa_inband_rcv_ll(skb, dev);
 			return NULL;
 		case DSA_CODE_ARP_MIRROR:
 		case DSA_CODE_POLICY_MIRROR:
@@ -325,6 +400,12 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
 
+static int dsa_inband_xmit(struct sk_buff *skb, struct net_device *dev,
+			   int seq_no)
+{
+	return dsa_inband_xmit_ll(skb, dev, NULL, 0, seq_no);
+}
+
 static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	return dsa_xmit_ll(skb, dev, 0);
@@ -343,6 +424,7 @@ static const struct dsa_device_ops dsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_DSA,
 	.xmit	  = dsa_xmit,
 	.rcv	  = dsa_rcv,
+	.inband_xmit = dsa_inband_xmit,
 	.needed_headroom = DSA_HLEN,
 };
 
@@ -354,6 +436,19 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_DSA);
 
 #define EDSA_HLEN 8
 
+static int edsa_inband_xmit(struct sk_buff *skb, struct net_device *dev,
+			    int seq_no)
+{
+	u8 edsa_header[4];
+
+	edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
+	edsa_header[1] = ETH_P_EDSA & 0xff;
+	edsa_header[2] = 0x00;
+	edsa_header[3] = 0x00;
+
+	return dsa_inband_xmit_ll(skb, dev, edsa_header, 4, seq_no);
+}
+
 static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u8 *edsa_header;
@@ -385,6 +480,7 @@ static const struct dsa_device_ops edsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_EDSA,
 	.xmit	  = edsa_xmit,
 	.rcv	  = edsa_rcv,
+	.inband_xmit = edsa_inband_xmit,
 	.needed_headroom = EDSA_HLEN,
 };
 
-- 
2.25.1

Re: [RFC net-next PATCH 1/3] dsa: Add ability to handle RMU frames.

[Andrew Lunn]

> +static int dsa_inband_rcv_ll(struct sk_buff *skb, struct net_device *dev)
> +{
> +	int source_device, source_port;
> +	struct dsa_switch *ds;
> +	u8 *dsa_header;
> +	int rcv_seqno;
> +	int ret = 0;
> +
> +	if (!dev || !dev->dsa_ptr)
> +		return 0;
> +
> +	ds = dev->dsa_ptr->ds;
> +
> +	dsa_header = skb->data - 2;
> +
> +	source_device = dsa_header[0] & 0x1f;
> +	source_port = (dsa_header[1] >> 3) & 0x1f;
> +	ds = dsa_switch_find(ds->dst->index, source_device);

You should never trust anything you receive from the network. Always
validate it. ds could be a NULL pointer here, if source_device is
bad. source_port could also be invalid. Hum, source port is not
actually used?

We send RMU frames with a specific destination MAC address. Can we
validate the destination address for frames we receive.

> +
> +	/* Get rcv seqno */
> +	rcv_seqno = dsa_header[3];
> +
> +	skb_pull(skb, DSA_HLEN);
> +
> +	if (ds->ops && ds->ops->inband_receive(ds, skb, rcv_seqno))
> +		netdev_err(dev, "DSA inband: error decoding packet");

rate limit this print, so as to avoid the possibility of a DoS.

     Andrew

Re: [RFC net-next PATCH 1/3] dsa: Add ability to handle RMU frames.

[Mattias Forsblad]

On 2022-08-18 14:44, Andrew Lunn wrote:
>> +static int dsa_inband_rcv_ll(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +	int source_device, source_port;
>> +	struct dsa_switch *ds;
>> +	u8 *dsa_header;
>> +	int rcv_seqno;
>> +	int ret = 0;
>> +
>> +	if (!dev || !dev->dsa_ptr)
>> +		return 0;
>> +
>> +	ds = dev->dsa_ptr->ds;
>> +
>> +	dsa_header = skb->data - 2;
>> +
>> +	source_device = dsa_header[0] & 0x1f;
>> +	source_port = (dsa_header[1] >> 3) & 0x1f;
>> +	ds = dsa_switch_find(ds->dst->index, source_device);
> 
> You should never trust anything you receive from the network. Always
> validate it. ds could be a NULL pointer here, if source_device is
> bad. source_port could also be invalid. Hum, source port is not
> actually used?
> 
Agree, will fix. I think source_port is a remnant from an earlier
version, I will fix it. 

> We send RMU frames with a specific destination MAC address. Can we
> validate the destination address for frames we receive.
>

Yes, I'll add that.
 
>> +
>> +	/* Get rcv seqno */
>> +	rcv_seqno = dsa_header[3];
>> +
>> +	skb_pull(skb, DSA_HLEN);
>> +
>> +	if (ds->ops && ds->ops->inband_receive(ds, skb, rcv_seqno))
>> +		netdev_err(dev, "DSA inband: error decoding packet");
> 
> rate limit this print, so as to avoid the possibility of a DoS.
> 
>      Andrew

Ofc, will fix. Thanks.

[RFC net-next PATCH 2/3] mv88e6xxx: Implement remote management support (RMU)

[Mattias Forsblad]

Implement support for handling RMU layer 3 frames
including receive and transmit.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/Makefile  |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c    |  19 ++-
 drivers/net/dsa/mv88e6xxx/chip.h    |  20 +++
 drivers/net/dsa/mv88e6xxx/global1.c |  84 +++++++++
 drivers/net/dsa/mv88e6xxx/global1.h |   3 +
 drivers/net/dsa/mv88e6xxx/rmu.c     | 256 ++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/rmu.h     |  18 ++
 7 files changed, 399 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.h

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index c8eca2b6f959..105d7bd832c9 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -15,3 +15,4 @@ mv88e6xxx-objs += port_hidden.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
 mv88e6xxx-objs += serdes.o
 mv88e6xxx-objs += smi.o
+mv88e6xxx-objs += rmu.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 07e9a4da924c..888c6e47dd16 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -42,6 +42,7 @@
 #include "ptp.h"
 #include "serdes.h"
 #include "smi.h"
+#include "rmu.h"
 
 static void assert_reg_lock(struct mv88e6xxx_chip *chip)
 {
@@ -1529,10 +1530,17 @@ static int mv88e6xxx_trunk_setup(struct mv88e6xxx_chip *chip)
 
 static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
 {
+	int ret = 0;
+
 	if (chip->info->ops->rmu_disable)
-		return chip->info->ops->rmu_disable(chip);
+		ret = chip->info->ops->rmu_disable(chip);
 
-	return 0;
+	if (chip->info->ops->rmu_enable) {
+		ret += chip->info->ops->rmu_enable(chip);
+		ret += mv88e6xxx_rmu_init(chip);
+	}
+
+	return ret;
 }
 
 static int mv88e6xxx_pot_setup(struct mv88e6xxx_chip *chip)
@@ -4090,6 +4098,7 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
 	.ppu_disable = mv88e6185_g1_ppu_disable,
 	.reset = mv88e6185_g1_reset,
 	.rmu_disable = mv88e6085_g1_rmu_disable,
+	.rmu_enable = mv88e6085_g1_rmu_enable,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.stu_getnext = mv88e6352_g1_stu_getnext,
@@ -4173,6 +4182,7 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6085_g1_rmu_disable,
+	.rmu_enable = mv88e6085_g1_rmu_enable,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.phylink_get_caps = mv88e6095_phylink_get_caps,
@@ -5292,6 +5302,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6352_g1_rmu_disable,
+	.rmu_enable = mv88e6352_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
@@ -5359,6 +5370,7 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6390_g1_rmu_disable,
+	.rmu_enable = mv88e6390_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
@@ -5426,6 +5438,7 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6390_g1_rmu_disable,
+	.rmu_enable = mv88e6390_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
@@ -5496,6 +5509,7 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6390_g1_rmu_disable,
+	.rmu_enable = mv88e6390_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
@@ -6918,6 +6932,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.crosschip_lag_change	= mv88e6xxx_crosschip_lag_change,
 	.crosschip_lag_join	= mv88e6xxx_crosschip_lag_join,
 	.crosschip_lag_leave	= mv88e6xxx_crosschip_lag_leave,
+	.inband_receive         = mv88e6xxx_inband_rcv,
 };
 
 static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e693154cf803..024f45cc1476 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -33,6 +33,8 @@
 
 #define MV88E6XXX_MAX_GPIO	16
 
+#define MV88E6XXX_WAIT_POLL_TIME_MS		200
+
 enum mv88e6xxx_egress_mode {
 	MV88E6XXX_EGRESS_MODE_UNMODIFIED,
 	MV88E6XXX_EGRESS_MODE_UNTAGGED,
@@ -266,6 +268,7 @@ struct mv88e6xxx_vlan {
 struct mv88e6xxx_port {
 	struct mv88e6xxx_chip *chip;
 	int port;
+	u64 rmu_raw_stats[64];
 	struct mv88e6xxx_vlan bridge_pvid;
 	u64 serdes_stats[2];
 	u64 atu_member_violation;
@@ -282,6 +285,18 @@ struct mv88e6xxx_port {
 	struct devlink_region *region;
 };
 
+struct mv88e6xxx_rmu {
+	/* RMU resources */
+	struct net_device *netdev;
+	struct mv88e6xxx_bus_ops *ops;
+	struct completion completion;
+	/* Mutex for RMU operations */
+	struct mutex mutex;
+	u16 got_id;
+	u8 request_cmd;
+	u8 seq_no;
+};
+
 enum mv88e6xxx_region_id {
 	MV88E6XXX_REGION_GLOBAL1 = 0,
 	MV88E6XXX_REGION_GLOBAL2,
@@ -410,12 +425,16 @@ struct mv88e6xxx_chip {
 
 	/* Bridge MST to SID mappings */
 	struct list_head msts;
+
+	/* RMU resources */
+	struct mv88e6xxx_rmu rmu;
 };
 
 struct mv88e6xxx_bus_ops {
 	int (*read)(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val);
 	int (*write)(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val);
 	int (*init)(struct mv88e6xxx_chip *chip);
+	int (*get_rmon)(struct mv88e6xxx_chip *chip, int port, uint64_t *data);
 };
 
 struct mv88e6xxx_mdio_bus {
@@ -637,6 +656,7 @@ struct mv88e6xxx_ops {
 
 	/* Remote Management Unit operations */
 	int (*rmu_disable)(struct mv88e6xxx_chip *chip);
+	int (*rmu_enable)(struct mv88e6xxx_chip *chip);
 
 	/* Precision Time Protocol operations */
 	const struct mv88e6xxx_ptp_ops *ptp_ops;
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
index 5848112036b0..ba756d918e13 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -466,18 +466,102 @@ int mv88e6085_g1_rmu_disable(struct mv88e6xxx_chip *chip)
 				      MV88E6085_G1_CTL2_RM_ENABLE, 0);
 }
 
+int mv88e6085_g1_rmu_enable(struct mv88e6xxx_chip *chip)
+{
+	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
+	int upstream_port = -1;
+
+	upstream_port = dsa_switch_upstream_port(chip->ds);
+	dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
+	if (upstream_port < 0)
+		return -1;
+
+	switch (upstream_port) {
+	case 9:
+		val = MV88E6085_G1_CTL2_RM_ENABLE;
+		break;
+	case 10:
+		val = MV88E6085_G1_CTL2_RM_ENABLE | MV88E6085_G1_CTL2_P10RM;
+		break;
+	default:
+		break;
+	}
+
+	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6085_G1_CTL2_P10RM |
+				      MV88E6085_G1_CTL2_RM_ENABLE, val);
+}
+
 int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6352_G1_CTL2_RMU_MODE_MASK,
 				      MV88E6352_G1_CTL2_RMU_MODE_DISABLED);
 }
 
+int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip)
+{
+	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
+	int upstream_port;
+
+	upstream_port = dsa_switch_upstream_port(chip->ds);
+	dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
+	if (upstream_port < 0)
+		return -1;
+
+	switch (upstream_port) {
+	case 4:
+		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_4;
+		break;
+	case 5:
+		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_5;
+		break;
+	case 6:
+		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_6;
+		break;
+	default:
+		break;
+	}
+
+	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6352_G1_CTL2_RMU_MODE_MASK,
+			val);
+}
+
 int mv88e6390_g1_rmu_disable(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6390_G1_CTL2_RMU_MODE_MASK,
 				      MV88E6390_G1_CTL2_RMU_MODE_DISABLED);
 }
 
+int mv88e6390_g1_rmu_enable(struct mv88e6xxx_chip *chip)
+{
+	int val = MV88E6390_G1_CTL2_RMU_MODE_DISABLED;
+	int upstream_port;
+
+	upstream_port = dsa_switch_upstream_port(chip->ds);
+	dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
+	if (upstream_port < 0)
+		return -1;
+
+	switch (upstream_port) {
+	case 0:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_0;
+		break;
+	case 1:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_1;
+		break;
+	case 9:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_9;
+		break;
+	case 10:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_10;
+		break;
+	default:
+		break;
+	}
+
+	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6390_G1_CTL2_RMU_MODE_MASK,
+			val);
+}
+
 int mv88e6390_g1_stats_set_histogram(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6390_G1_CTL2_HIST_MODE_MASK,
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 65958b2a0d3a..7e786503734a 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -313,8 +313,11 @@ int mv88e6250_g1_ieee_pri_map(struct mv88e6xxx_chip *chip);
 int mv88e6185_g1_set_cascade_port(struct mv88e6xxx_chip *chip, int port);
 
 int mv88e6085_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6085_g1_rmu_enable(struct mv88e6xxx_chip *chip);
 int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip);
 int mv88e6390_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6390_g1_rmu_enable(struct mv88e6xxx_chip *chip);
 
 int mv88e6xxx_g1_set_device_number(struct mv88e6xxx_chip *chip, int index);
 
diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
new file mode 100644
index 000000000000..ac68eef12521
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/rmu.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Marvell 88E6xxx Switch Remote Management Unit Support
+ *
+ * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
+ *
+ */
+
+#include "rmu.h"
+#include "global1.h"
+
+#define MAX_RMON 64
+#define RMON_REPLY 2
+
+#define RMU_REQ_GET_ID                 1
+#define RMU_REQ_DUMP_MIB               2
+
+#define RMU_RESP_FORMAT_1              0x0001
+#define RMU_RESP_FORMAT_2              0x0002
+
+#define RMU_RESP_CODE_GOT_ID           0x0000
+#define RMU_RESP_CODE_DUMP_MIB         0x1020
+
+int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port *port;
+	__be16 *prodnum;
+	__be16 *format;
+	__be16 *code;
+	__be32 *mib_data;
+	u8 pkt_dev;
+	u8 pkt_prt;
+	int i;
+
+	if (!skb || !chip)
+		return 0;
+
+	/* Extract response data */
+	format = (__be16 *)&skb->data[0];
+	if (*format != htons(RMU_RESP_FORMAT_1) && (*format != htons(RMU_RESP_FORMAT_2))) {
+		dev_err(chip->dev, "RMU: received unknown format 0x%04x", *format);
+		goto out;
+	}
+
+	code = (__be16 *)&skb->data[4];
+	if (*code == ntohs(0xffff)) {
+		netdev_err(skb->dev, "RMU: error response code 0x%04x", *code);
+		goto out;
+	}
+
+	pkt_dev = skb->data[6] & 0x1f;
+	if (pkt_dev >= DSA_MAX_SWITCHES) {
+		netdev_err(skb->dev, "RMU: response from unknown chip %d\n", *code);
+		goto out;
+	}
+
+	/* Check sequence number */
+	if (seq_no != chip->rmu.seq_no) {
+		netdev_err(skb->dev, "RMU: wrong seqno received %d, expected %d",
+			   seq_no, chip->rmu.seq_no);
+		goto out;
+	}
+
+	/* Check response code */
+	switch (chip->rmu.request_cmd) {
+	case RMU_REQ_GET_ID: {
+		if (*code == RMU_RESP_CODE_GOT_ID) {
+			prodnum = (__be16 *)&skb->data[2];
+			chip->rmu.got_id = *prodnum;
+			dev_info(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
+				 chip->rmu.got_id);
+		} else {
+			dev_err(chip->dev,
+				"RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
+				*format, *code);
+		}
+		break;
+	}
+	case RMU_REQ_DUMP_MIB:
+		if (*code == RMU_RESP_CODE_DUMP_MIB) {
+			pkt_prt = (skb->data[7] & 0x78) >> 3;
+			mib_data = (__be32 *)&skb->data[12];
+			port = &chip->ports[pkt_prt];
+			if (!port) {
+				dev_err(chip->dev, "RMU: illegal port number in response: %d\n",
+					pkt_prt);
+				goto out;
+			}
+
+			/* Copy whole array for further
+			 * processing according to chip type
+			 */
+			for (i = 0; i < MAX_RMON; i++)
+				port->rmu_raw_stats[i] = mib_data[i];
+		}
+		break;
+	default:
+		dev_err(chip->dev,
+			"RMU: unknown response format 0x%04x and code 0x%04x from chip %d\n",
+			*format, *code, chip->ds->index);
+		break;
+	}
+
+out:
+	complete(&chip->rmu.completion);
+
+	return 0;
+}
+
+static int mv88e6xxx_rmu_tx(struct mv88e6xxx_chip *chip, int port,
+			    const char *msg, int len)
+{
+	const struct dsa_device_ops *tag_ops;
+	const struct dsa_port *dp;
+	unsigned char *data;
+	struct sk_buff *skb;
+
+	dp = dsa_to_port(chip->ds, port);
+	if (!dp || !dp->cpu_dp)
+		return 0;
+
+	tag_ops = dp->cpu_dp->tag_ops;
+	if (!tag_ops)
+		return -ENODEV;
+
+	skb = netdev_alloc_skb(chip->rmu.netdev, 64);
+	if (!skb)
+		return -ENOMEM;
+
+	skb_reserve(skb, 2 * ETH_HLEN + tag_ops->needed_headroom);
+	skb_reset_network_header(skb);
+	skb->pkt_type = PACKET_OUTGOING;
+	skb->dev = chip->rmu.netdev;
+
+	/* Create RMU L3 message */
+	data = skb_put(skb, len);
+	memcpy(data, msg, len);
+
+	return tag_ops->inband_xmit(skb, dp->slave, ++chip->rmu.seq_no);
+}
+
+static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip, int port,
+				   int request, const char *msg, int len)
+{
+	const struct dsa_port *dp;
+	struct net_device *master;
+	int ret = 0;
+
+	dp = dsa_to_port(chip->ds, port);
+	if (!dp)
+		return 0;
+
+	master = dp->master;
+
+	mutex_lock(&chip->rmu.mutex);
+
+	chip->rmu.request_cmd = request;
+
+	ret = mv88e6xxx_rmu_tx(chip, port, msg, len);
+	if (ret == -ENODEV) {
+		/* Device not ready yet? Try again later */
+		ret = 0;
+		goto out;
+	}
+
+	if (ret) {
+		dev_err(chip->dev, "RMU: error transmitting request (%d)", ret);
+		goto out;
+	}
+
+	ret = wait_for_completion_timeout(&chip->rmu.completion,
+					  msecs_to_jiffies(MV88E6XXX_WAIT_POLL_TIME_MS));
+	if (ret == 0) {
+		dev_err(chip->dev,
+			"RMU: timeout waiting for request %d (%d) on dev:port %d:%d\n",
+			request, ret, chip->ds->index, port);
+		ret = -ETIMEDOUT;
+	}
+
+out:
+	mutex_unlock(&chip->rmu.mutex);
+
+	return ret > 0 ? 0 : ret;
+}
+
+static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
+{
+	const u8 get_id[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+	int ret = -1;
+
+	if (chip->rmu.got_id)
+		return 0;
+
+	chip->rmu.netdev = dev_get_by_name(&init_net, "chan0");
+	if (!chip->rmu.netdev) {
+		dev_err(chip->dev, "RMU: unable to get interface");
+		return -ENODEV;
+	}
+
+	ret = mv88e6xxx_rmu_send_wait(chip, port, RMU_REQ_GET_ID, get_id, 8);
+	if (ret) {
+		dev_err(chip->dev, "RMU: error for command GET_ID %d index %d\n", ret,
+			chip->ds->index);
+		return ret;
+	}
+
+	return 0;
+}
+
+int mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
+{
+	u8 dump_mib[8] = { 0x00, 0x01, 0x00, 0x00, 0x10, 0x20, 0x00, 0x00 };
+	int ret;
+
+	if (!chip)
+		return 0;
+
+	ret = mv88e6xxx_rmu_get_id(chip, port);
+	if (ret)
+		return ret;
+
+	/* Send a GET_MIB command */
+	dump_mib[7] = port;
+	ret = mv88e6xxx_rmu_send_wait(chip, port, RMU_REQ_DUMP_MIB, dump_mib, 8);
+	if (ret) {
+		dev_err(chip->dev, "RMU: error for command DUMP_MIB %d dev %d:%d\n", ret,
+			chip->ds->index, port);
+		return ret;
+	}
+
+	/* Update MIB for port */
+	if (chip->info->ops->stats_get_stats)
+		return chip->info->ops->stats_get_stats(chip, port, data);
+
+	return 0;
+}
+
+static struct mv88e6xxx_bus_ops mv88e6xxx_bus_ops = {
+	.get_rmon = mv88e6xxx_rmu_stats_get,
+};
+
+int mv88e6xxx_rmu_init(struct mv88e6xxx_chip *chip)
+{
+	int ret = 0;
+
+	dev_info(chip->dev, "RMU: setting up for switch@%d", chip->sw_addr);
+
+	init_completion(&chip->rmu.completion);
+
+	mutex_init(&chip->rmu.mutex);
+
+	chip->rmu.ops = &mv88e6xxx_bus_ops;
+
+	return ret;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/rmu.h b/drivers/net/dsa/mv88e6xxx/rmu.h
new file mode 100644
index 000000000000..3f74e952cad9
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/rmu.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Marvell 88E6xxx Switch Remote Management Unit Support
+ *
+ * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
+ *
+ */
+
+#ifndef _MV88E6XXX_RMU_H_
+#define _MV88E6XXX_RMU_H_
+
+#include "chip.h"
+
+int mv88e6xxx_rmu_init(struct mv88e6xxx_chip *chip);
+
+int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no);
+
+#endif /* _MV88E6XXX_RMU_H_ */
-- 
2.25.1

Re: [RFC net-next PATCH 2/3] mv88e6xxx: Implement remote management support (RMU)

[Andrew Lunn]

>  static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>  {
> +	int ret = 0;
> +
>  	if (chip->info->ops->rmu_disable)
> -		return chip->info->ops->rmu_disable(chip);
> +		ret = chip->info->ops->rmu_disable(chip);
>  
> -	return 0;
> +	if (chip->info->ops->rmu_enable) {
> +		ret += chip->info->ops->rmu_enable(chip);
> +		ret += mv88e6xxx_rmu_init(chip);

EINVAL + EOPNOTSUPP = hard to find error code/bug.

I've not looked at rmu_enable() yet, but there are restrictions about
what ports can be used with RMU. So you have to assume some boards use
the wrong port for the CPU or upstream DSA port, and so will need to
return an error code. But such an error code should not be fatal, MDIO
can still be used.

> +	}
> +
> +	return ret;
>  }
>  
> +int mv88e6085_g1_rmu_enable(struct mv88e6xxx_chip *chip)
> +{
> +	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
> +	int upstream_port = -1;
> +
> +	upstream_port = dsa_switch_upstream_port(chip->ds);
> +	dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);

dev_dbg()

> +	if (upstream_port < 0)
> +		return -1;

EOPNOTSUPP.

> +
> +	switch (upstream_port) {
> +	case 9:
> +		val = MV88E6085_G1_CTL2_RM_ENABLE;
> +		break;
> +	case 10:
> +		val = MV88E6085_G1_CTL2_RM_ENABLE | MV88E6085_G1_CTL2_P10RM;
> +		break;

Does 6085 really have 10 ports? It does!

> +	default:
> +		break;

return -EOPNOTSUPP.

> +	}
> +
> +	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6085_G1_CTL2_P10RM |
> +				      MV88E6085_G1_CTL2_RM_ENABLE, val);
> +}
> +
>  int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip)
>  {
>  	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6352_G1_CTL2_RMU_MODE_MASK,
>  				      MV88E6352_G1_CTL2_RMU_MODE_DISABLED);
>  }
>  
> +int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip)
> +{
> +	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
> +	int upstream_port;
> +
> +	upstream_port = dsa_switch_upstream_port(chip->ds);
> +	dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
> +	if (upstream_port < 0)
> +		return -1;
> +
> +	switch (upstream_port) {
> +	case 4:
> +		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_4;
> +		break;
> +	case 5:
> +		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_5;
> +		break;
> +	case 6:
> +		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_6;
> +		break;
> +	default:
> +		break;

Same comments as above.


> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
> new file mode 100644
> index 000000000000..ac68eef12521
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell 88E6xxx Switch Remote Management Unit Support
> + *
> + * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
> + *
> + */
> +
> +#include "rmu.h"
> +#include "global1.h"
> +
> +#define MAX_RMON 64
> +#define RMON_REPLY 2
> +
> +#define RMU_REQ_GET_ID                 1
> +#define RMU_REQ_DUMP_MIB               2
> +
> +#define RMU_RESP_FORMAT_1              0x0001
> +#define RMU_RESP_FORMAT_2              0x0002
> +
> +#define RMU_RESP_CODE_GOT_ID           0x0000
> +#define RMU_RESP_CODE_DUMP_MIB         0x1020

These should all go into rmu.h. Please also follow the naming
convention, add the MV88E6XXX_ prefix.


> +
> +int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_port *port;
> +	__be16 *prodnum;
> +	__be16 *format;
> +	__be16 *code;
> +	__be32 *mib_data;
> +	u8 pkt_dev;
> +	u8 pkt_prt;
> +	int i;

Reverse christmass tree.

> +
> +	if (!skb || !chip)
> +		return 0;

We generally don't do this sort of defensive programming. Can these be
NULL?

> +
> +	/* Extract response data */
> +	format = (__be16 *)&skb->data[0];

You have no idea of the alignment of data, so you should not cast it
to a pointer type and dereference it. Take a look at the unaligned
helpers.

> +	if (*format != htons(RMU_RESP_FORMAT_1) && (*format != htons(RMU_RESP_FORMAT_2))) {
> +		dev_err(chip->dev, "RMU: received unknown format 0x%04x", *format);

rate limit all error messages please.

> +		goto out;
> +	}
> +
> +	code = (__be16 *)&skb->data[4];
> +	if (*code == ntohs(0xffff)) {
> +		netdev_err(skb->dev, "RMU: error response code 0x%04x", *code);
> +		goto out;
> +	}
> +
> +	pkt_dev = skb->data[6] & 0x1f;

Please replace all these magic numbers with #define in rmu.h.

> +	if (pkt_dev >= DSA_MAX_SWITCHES) {
> +		netdev_err(skb->dev, "RMU: response from unknown chip %d\n", *code);
> +		goto out;
> +	}

That is a good first step, but it is not sufficient to prove the
switch actually exists.

> +
> +	/* Check sequence number */
> +	if (seq_no != chip->rmu.seq_no) {
> +		netdev_err(skb->dev, "RMU: wrong seqno received %d, expected %d",
> +			   seq_no, chip->rmu.seq_no);
> +		goto out;
> +	}
> +
> +	/* Check response code */
> +	switch (chip->rmu.request_cmd) {

Maybe a non-issue, i've not looked hard enough: What is the
relationship between ds passed to this function and pkt_dev? Does ds
belong to pkt_dev, or is ds the chip connected to the host? There are
a few boards with multiple chip in a tree, so we need to get this
mapping correct. This is something i can test sometime later, i have
such boards.

> +	case RMU_REQ_GET_ID: {
> +		if (*code == RMU_RESP_CODE_GOT_ID) {
> +			prodnum = (__be16 *)&skb->data[2];
> +			chip->rmu.got_id = *prodnum;
> +			dev_info(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
> +				 chip->rmu.got_id);
> +		} else {
> +			dev_err(chip->dev,
> +				"RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
> +				*format, *code);
> +		}
> +		break;
> +	}
> +	case RMU_REQ_DUMP_MIB:
> +		if (*code == RMU_RESP_CODE_DUMP_MIB) {
> +			pkt_prt = (skb->data[7] & 0x78) >> 3;
> +			mib_data = (__be32 *)&skb->data[12];
> +			port = &chip->ports[pkt_prt];
> +			if (!port) {
> +				dev_err(chip->dev, "RMU: illegal port number in response: %d\n",
> +					pkt_prt);
> +				goto out;
> +			}
> +
> +			/* Copy whole array for further
> +			 * processing according to chip type
> +			 */
> +			for (i = 0; i < MAX_RMON; i++)
> +				port->rmu_raw_stats[i] = mib_data[i];

This needs some more thought, which i don't have time for right now.

     Andrew

Re: [RFC net-next PATCH 2/3] mv88e6xxx: Implement remote management support (RMU)

[Mattias Forsblad]

On 2022-08-18 15:21, Andrew Lunn wrote:
>>  static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>>  {
>> +	int ret = 0;
>> +
>>  	if (chip->info->ops->rmu_disable)
>> -		return chip->info->ops->rmu_disable(chip);
>> +		ret = chip->info->ops->rmu_disable(chip);
>>  
>> -	return 0;
>> +	if (chip->info->ops->rmu_enable) {
>> +		ret += chip->info->ops->rmu_enable(chip);
>> +		ret += mv88e6xxx_rmu_init(chip);
> 
> EINVAL + EOPNOTSUPP = hard to find error code/bug.
> 
> I've not looked at rmu_enable() yet, but there are restrictions about
> what ports can be used with RMU. So you have to assume some boards use
> the wrong port for the CPU or upstream DSA port, and so will need to
> return an error code. But such an error code should not be fatal, MDIO
> can still be used.
> 

Agree, I'll fix to handle that case.

>> +	}
>> +
>> +	return ret;
>>  }
>>  
>> +int mv88e6085_g1_rmu_enable(struct mv88e6xxx_chip *chip)
>> +{
>> +	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
>> +	int upstream_port = -1;
>> +
>> +	upstream_port = dsa_switch_upstream_port(chip->ds);
>> +	dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
> 
> dev_dbg()

I'll tone it down.

> 
>> +	if (upstream_port < 0)
>> +		return -1;
> 
> EOPNOTSUPP.
> 

Ofc. Thanks.

>> +
>> +	switch (upstream_port) {
>> +	case 9:
>> +		val = MV88E6085_G1_CTL2_RM_ENABLE;
>> +		break;
>> +	case 10:
>> +		val = MV88E6085_G1_CTL2_RM_ENABLE | MV88E6085_G1_CTL2_P10RM;
>> +		break;
> 
> Does 6085 really have 10 ports? It does!
> 

:)

>> +	default:
>> +		break;
> 
> return -EOPNOTSUPP.
> 

Will fix.

>> +	}
>> +
>> +	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6085_G1_CTL2_P10RM |
>> +				      MV88E6085_G1_CTL2_RM_ENABLE, val);
>> +}
>> +
>>  int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip)
>>  {
>>  	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6352_G1_CTL2_RMU_MODE_MASK,
>>  				      MV88E6352_G1_CTL2_RMU_MODE_DISABLED);
>>  }
>>  
>> +int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip)
>> +{
>> +	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
>> +	int upstream_port;
>> +
>> +	upstream_port = dsa_switch_upstream_port(chip->ds);
>> +	dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
>> +	if (upstream_port < 0)
>> +		return -1;
>> +
>> +	switch (upstream_port) {
>> +	case 4:
>> +		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_4;
>> +		break;
>> +	case 5:
>> +		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_5;
>> +		break;
>> +	case 6:
>> +		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_6;
>> +		break;
>> +	default:
>> +		break;
> 
> Same comments as above.
> 
> 
>> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
>> new file mode 100644
>> index 000000000000..ac68eef12521
>> --- /dev/null
>> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
>> @@ -0,0 +1,256 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Marvell 88E6xxx Switch Remote Management Unit Support
>> + *
>> + * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
>> + *
>> + */
>> +
>> +#include "rmu.h"
>> +#include "global1.h"
>> +
>> +#define MAX_RMON 64
>> +#define RMON_REPLY 2
>> +
>> +#define RMU_REQ_GET_ID                 1
>> +#define RMU_REQ_DUMP_MIB               2
>> +
>> +#define RMU_RESP_FORMAT_1              0x0001
>> +#define RMU_RESP_FORMAT_2              0x0002
>> +
>> +#define RMU_RESP_CODE_GOT_ID           0x0000
>> +#define RMU_RESP_CODE_DUMP_MIB         0x1020
> 
> These should all go into rmu.h. Please also follow the naming
> convention, add the MV88E6XXX_ prefix.
> 
> 

Will add prefix.

>> +
>> +int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
>> +{
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +	struct mv88e6xxx_port *port;
>> +	__be16 *prodnum;
>> +	__be16 *format;
>> +	__be16 *code;
>> +	__be32 *mib_data;
>> +	u8 pkt_dev;
>> +	u8 pkt_prt;
>> +	int i;
> 
> Reverse christmass tree.
> 

Will fix. Doesn't checkpatch find these?

>> +
>> +	if (!skb || !chip)
>> +		return 0;
> 
> We generally don't do this sort of defensive programming. Can these be
> NULL?
> 

Will investigate.

>> +
>> +	/* Extract response data */
>> +	format = (__be16 *)&skb->data[0];
> 
> You have no idea of the alignment of data, so you should not cast it
> to a pointer type and dereference it. Take a look at the unaligned
> helpers.
> 

Can you point me to an example please?

>> +	if (*format != htons(RMU_RESP_FORMAT_1) && (*format != htons(RMU_RESP_FORMAT_2))) {
>> +		dev_err(chip->dev, "RMU: received unknown format 0x%04x", *format);
> 
> rate limit all error messages please.
> 

Yes, will fix.

>> +		goto out;
>> +	}
>> +
>> +	code = (__be16 *)&skb->data[4];
>> +	if (*code == ntohs(0xffff)) {
>> +		netdev_err(skb->dev, "RMU: error response code 0x%04x", *code);
>> +		goto out;
>> +	}
>> +
>> +	pkt_dev = skb->data[6] & 0x1f;
> 
> Please replace all these magic numbers with #define in rmu.h.
> 

Will fix.

>> +	if (pkt_dev >= DSA_MAX_SWITCHES) {
>> +		netdev_err(skb->dev, "RMU: response from unknown chip %d\n", *code);
>> +		goto out;
>> +	}
> 
> That is a good first step, but it is not sufficient to prove the
> switch actually exists.
> 

Will do additional checks.

>> +
>> +	/* Check sequence number */
>> +	if (seq_no != chip->rmu.seq_no) {
>> +		netdev_err(skb->dev, "RMU: wrong seqno received %d, expected %d",
>> +			   seq_no, chip->rmu.seq_no);
>> +		goto out;
>> +	}
>> +
>> +	/* Check response code */
>> +	switch (chip->rmu.request_cmd) {
> 
> Maybe a non-issue, i've not looked hard enough: What is the
> relationship between ds passed to this function and pkt_dev? Does ds
> belong to pkt_dev, or is ds the chip connected to the host? There are
> a few boards with multiple chip in a tree, so we need to get this
> mapping correct. This is something i can test sometime later, i have
> such boards.
> 

I've tested this implementation in a system with multiple switchcores
and it works.

>> +	case RMU_REQ_GET_ID: {
>> +		if (*code == RMU_RESP_CODE_GOT_ID) {
>> +			prodnum = (__be16 *)&skb->data[2];
>> +			chip->rmu.got_id = *prodnum;
>> +			dev_info(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
>> +				 chip->rmu.got_id);
>> +		} else {
>> +			dev_err(chip->dev,
>> +				"RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
>> +				*format, *code);
>> +		}
>> +		break;
>> +	}
>> +	case RMU_REQ_DUMP_MIB:
>> +		if (*code == RMU_RESP_CODE_DUMP_MIB) {
>> +			pkt_prt = (skb->data[7] & 0x78) >> 3;
>> +			mib_data = (__be32 *)&skb->data[12];
>> +			port = &chip->ports[pkt_prt];
>> +			if (!port) {
>> +				dev_err(chip->dev, "RMU: illegal port number in response: %d\n",
>> +					pkt_prt);
>> +				goto out;
>> +			}
>> +
>> +			/* Copy whole array for further
>> +			 * processing according to chip type
>> +			 */
>> +			for (i = 0; i < MAX_RMON; i++)
>> +				port->rmu_raw_stats[i] = mib_data[i];
> 
> This needs some more thought, which i don't have time for right now.
> 
>      Andrew

Re: [RFC net-next PATCH 2/3] mv88e6xxx: Implement remote management support (RMU)

[Andrew Lunn]

> >> +int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
> >> +{
> >> +	struct mv88e6xxx_chip *chip = ds->priv;
> >> +	struct mv88e6xxx_port *port;
> >> +	__be16 *prodnum;
> >> +	__be16 *format;
> >> +	__be16 *code;
> >> +	__be32 *mib_data;
> >> +	u8 pkt_dev;
> >> +	u8 pkt_prt;
> >> +	int i;
> > 
> > Reverse christmass tree.
> > 
> 
> Will fix. Doesn't checkpatch find these?

No it does not :-(

It is only something which netdev enforces.

> >> +
> >> +	/* Extract response data */
> >> +	format = (__be16 *)&skb->data[0];
> > 
> > You have no idea of the alignment of data, so you should not cast it
> > to a pointer type and dereference it. Take a look at the unaligned
> > helpers.
> > 
> 
> Can you point me to an example please?

#include <asm/unaligned.h>

then you can use functions like get_unaligned_be16(),
get_unaligned_le32(), put_unaligned_le32().

On X86 unaligned accesses are cheap, so these macros do nothing. For
ARM they are expensive so split it into byte accesses.

> I've tested this implementation in a system with multiple switchcores
> and it works.

Cool, there are not many such boards, but RMU really helps these if
they are sharing the same MDIO bus.

     Andrew

[RFC net-next PATCH 3/3] mv88e6xxx: rmon: Use RMU to collect rmon data.

[Mattias Forsblad]

If RMU is supported, use that interface to
collect rmon data.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 41 +++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 888c6e47dd16..344d6633ad6d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1226,16 +1226,29 @@ static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				     u16 bank1_select, u16 histogram)
 {
 	struct mv88e6xxx_hw_stat *stat;
+	int offset = 0;
+	u64 high;
 	int i, j;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
 		stat = &mv88e6xxx_hw_stats[i];
 		if (stat->type & types) {
-			mv88e6xxx_reg_lock(chip);
-			data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
-							      bank1_select,
-							      histogram);
-			mv88e6xxx_reg_unlock(chip);
+			if (chip->rmu.ops->get_rmon && !(stat->type & STATS_TYPE_PORT)) {
+				if (stat->type & STATS_TYPE_BANK1)
+					offset = 32;
+
+				data[j] = chip->ports[port].rmu_raw_stats[stat->reg + offset];
+				if (stat->size == 8) {
+					high = chip->ports[port].rmu_raw_stats[stat->reg + offset
+							+ 1];
+					data[j] += (high << 32);
+				}
+			} else {
+				mv88e6xxx_reg_lock(chip);
+				data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
+								      bank1_select, histogram);
+				mv88e6xxx_reg_unlock(chip);
+			}
 
 			j++;
 		}
@@ -1310,16 +1323,22 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int ret;
 
-	mv88e6xxx_reg_lock(chip);
+	if (chip->rmu.ops && chip->rmu.ops->get_rmon) {
+		ret = chip->rmu.ops->get_rmon(chip, port, data);
+		if (ret == -ETIMEDOUT)
+			return;
+	} else {
 
-	ret = mv88e6xxx_stats_snapshot(chip, port);
-	mv88e6xxx_reg_unlock(chip);
+		mv88e6xxx_reg_lock(chip);
 
-	if (ret < 0)
-		return;
+		ret = mv88e6xxx_stats_snapshot(chip, port);
+		mv88e6xxx_reg_unlock(chip);
 
-	mv88e6xxx_get_stats(chip, port, data);
+		if (ret < 0)
+			return;
 
+		mv88e6xxx_get_stats(chip, port, data);
+	}
 }
 
 static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
-- 
2.25.1

Re: [RFC net-next PATCH 3/3] mv88e6xxx: rmon: Use RMU to collect rmon data.

[Andrew Lunn]

> @@ -1310,16 +1323,22 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int ret;
>  
> -	mv88e6xxx_reg_lock(chip);
> +	if (chip->rmu.ops && chip->rmu.ops->get_rmon) {
> +		ret = chip->rmu.ops->get_rmon(chip, port, data);
> +		if (ret == -ETIMEDOUT)
> +			return;
> +	} else {
>  
> -	ret = mv88e6xxx_stats_snapshot(chip, port);
> -	mv88e6xxx_reg_unlock(chip);
> +		mv88e6xxx_reg_lock(chip);
>  
> -	if (ret < 0)
> -		return;
> +		ret = mv88e6xxx_stats_snapshot(chip, port);
> +		mv88e6xxx_reg_unlock(chip);
>  
> -	mv88e6xxx_get_stats(chip, port, data);
> +		if (ret < 0)
> +			return;
>  
> +		mv88e6xxx_get_stats(chip, port, data);
> +	}
>  }

I don't particularly like the way this is all mixed together.

Could you try to split it, so there is an MDIO set of functions and an
RMU set of functions. Maybe you have some helpers which are used by
both.

I would also suggest you try to think about ATU dump and VTU dump. You
ideally want a code structure which is very similar for all these dump
operations. Take a look at how qca8k-8xxx.c does things.

Is it documented in the datasheet that when RMU is used a snapshot is
not required?

    Andrew

Re: [RFC net-next PATCH 3/3] mv88e6xxx: rmon: Use RMU to collect rmon data.

[Mattias Forsblad]

On 2022-08-18 17:36, Andrew Lunn wrote:
>> @@ -1310,16 +1323,22 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>>  	struct mv88e6xxx_chip *chip = ds->priv;
>>  	int ret;
>>  
>> -	mv88e6xxx_reg_lock(chip);
>> +	if (chip->rmu.ops && chip->rmu.ops->get_rmon) {
>> +		ret = chip->rmu.ops->get_rmon(chip, port, data);
>> +		if (ret == -ETIMEDOUT)
>> +			return;
>> +	} else {
>>  
>> -	ret = mv88e6xxx_stats_snapshot(chip, port);
>> -	mv88e6xxx_reg_unlock(chip);
>> +		mv88e6xxx_reg_lock(chip);
>>  
>> -	if (ret < 0)
>> -		return;
>> +		ret = mv88e6xxx_stats_snapshot(chip, port);
>> +		mv88e6xxx_reg_unlock(chip);
>>  
>> -	mv88e6xxx_get_stats(chip, port, data);
>> +		if (ret < 0)
>> +			return;
>>  
>> +		mv88e6xxx_get_stats(chip, port, data);
>> +	}
>>  }
> 
> I don't particularly like the way this is all mixed together.
> 
> Could you try to split it, so there is an MDIO set of functions and an
> RMU set of functions. Maybe you have some helpers which are used by
> both.
> 

I think that a great idea. Will split in separate functions.

> I would also suggest you try to think about ATU dump and VTU dump. You
> ideally want a code structure which is very similar for all these dump
> operations. Take a look at how qca8k-8xxx.c does things.
> 

I think that the ATU dump is the logical next step to implement. I'll
have a look in qca8k to see how they do things.

AFAIK there is no RMU operation for VTU dump. That needs to be done 
with register reads and writes.

> Is it documented in the datasheet that when RMU is used a snapshot is
> not required?
> 
>     Andrew

In the documentation they only mention captured counters with regards 
to MDIO accesses. I cannot see that that is necessary with RMU.
FYI on our board the time delta between request and reply 
is ~100us (which is nice)

Re: [RFC net-next PATCH 0/3] net: dsa: mv88e6xxx: Add RMU support

[Vladimir Oltean]

On Thu, Aug 18, 2022 at 12:29:21PM +0200, Mattias Forsblad wrote:
> The Marvell SOHO switches have the ability to receive and transmit
> Remote Management Frames (Frame2Reg) to the CPU through the
> switch attached network interface.
> These frames is handled by the Remote Management Unit (RMU) in
> the switch.
> These frames can contain different payloads:
> single switch register read and writes, daisy chained switch
> register read and writes, RMON/MIB dump/dump clear and ATU dump.
> The dump functions are very costly over MDIO but it's
> only a couple of network packets via the RMU. Handling these
> operations via RMU instead of MDIO also relieves access
> contention on the MDIO bus.
>
> This request for comment series implements RMU layer 2 and
> layer 3 handling and also collecting RMON counters
> through the RMU.
>
> Next step could be to implement single read and writes but we've
> found that the gain to transfer this to RMU is neglible.
>
> Regards,
> Mattias Forsblad

Have you seen how things work with qca8k_connect_tag_protocol()/
qca8k_master_change()/qca8k_read_eth()/qca8k_write_eth()/
qca8k_phy_eth_command()?

Re: [RFC net-next PATCH 0/3] net: dsa: mv88e6xxx: Add RMU support

[Mattias Forsblad]

On 2022-08-18 13:58, Vladimir Oltean wrote:
> On Thu, Aug 18, 2022 at 12:29:21PM +0200, Mattias Forsblad wrote:
>> The Marvell SOHO switches have the ability to receive and transmit
>> Remote Management Frames (Frame2Reg) to the CPU through the
>> switch attached network interface.
>> These frames is handled by the Remote Management Unit (RMU) in
>> the switch.
>> These frames can contain different payloads:
>> single switch register read and writes, daisy chained switch
>> register read and writes, RMON/MIB dump/dump clear and ATU dump.
>> The dump functions are very costly over MDIO but it's
>> only a couple of network packets via the RMU. Handling these
>> operations via RMU instead of MDIO also relieves access
>> contention on the MDIO bus.
>>
>> This request for comment series implements RMU layer 2 and
>> layer 3 handling and also collecting RMON counters
>> through the RMU.
>>
>> Next step could be to implement single read and writes but we've
>> found that the gain to transfer this to RMU is neglible.
>>
>> Regards,
>> Mattias Forsblad
> 
> Have you seen how things work with qca8k_connect_tag_protocol()/
> qca8k_master_change()/qca8k_read_eth()/qca8k_write_eth()/
> qca8k_phy_eth_command()?

No, I have not. I'll take a look at those. Thanks.

Re: [RFC net-next PATCH 0/3] net: dsa: mv88e6xxx: Add RMU support

[Andrew Lunn]

On Thu, Aug 18, 2022 at 12:29:21PM +0200, Mattias Forsblad wrote:
> The Marvell SOHO switches have the ability to receive and transmit                                                                                                    
> Remote Management Frames (Frame2Reg) to the CPU through the                                                                                                           
> switch attached network interface.                                                                                                                                    
> These frames is handled by the Remote Management Unit (RMU) in                                                                                                        
> the switch.

Please try to avoid all the additional whitespace your editor/mailer
has added.

> Next step could be to implement single read and writes but we've
> found that the gain to transfer this to RMU is neglible.

I agree that RMON is a good first step. Dumping the ATU and VTU would
also make a lot of sense.

For general register access, did you try combining multiple writes and
one read into an RMU packet? At least during initial setup, i suspect
there are some code flows which follow that pattern with lots of
writes. And a collection of read/modify/write might benefit.

    Andrew

Re: [RFC net-next PATCH 0/3] net: dsa: mv88e6xxx: Add RMU support

[Mattias Forsblad]

On 2022-08-18 14:31, Andrew Lunn wrote:
> On Thu, Aug 18, 2022 at 12:29:21PM +0200, Mattias Forsblad wrote:
>> The Marvell SOHO switches have the ability to receive and transmit                                                                                                    
>> Remote Management Frames (Frame2Reg) to the CPU through the                                                                                                           
>> switch attached network interface.                                                                                                                                    
>> These frames is handled by the Remote Management Unit (RMU) in                                                                                                        
>> the switch.
> 
> Please try to avoid all the additional whitespace your editor/mailer
> has added.
> 
>> Next step could be to implement single read and writes but we've
>> found that the gain to transfer this to RMU is neglible.
> 
> I agree that RMON is a good first step. Dumping the ATU and VTU would
> also make a lot of sense.
> 
> For general register access, did you try combining multiple writes and
> one read into an RMU packet? At least during initial setup, i suspect
> there are some code flows which follow that pattern with lots of
> writes. And a collection of read/modify/write might benefit.
> 
>     Andrew

In another stack I've used aggregated writes with great improvements so
it that is something that could be investigated. One large oversight when
implementing RMU in HW there is no operation for masked writes which makes
it a bit trickier. It would be great if there was a transaction-based
API which would easier map for aggregated accesses.
Oltean mentioned something about qck8k that I'll have a look into.