On Tue, Jan 25, 2022 at 07:48:27PM -0800, Florian Fainelli wrote:

On 1/22/2022 5:33 PM, Ansuel Smith wrote:

quoted

mgmt Ethernet packet can read/write up to 16byte at times. The len reg
is limited to 15 (0xf). The switch actually sends and accepts data in 4
different steps of len values.
Len steps:
- 0: nothing
- 1-4: first 4 byte
- 5-6: first 12 byte
- 7-15: all 16 byte

This is really odd, it almost felt like the length was a byte enable
bitmask, but it is not?

To me it seems like they match the size to the 3 different operation
that is
4: normal mdio / reg access
12: acl table to directly write and read it
16: offload table that is 16 byte long to directl write and read.

With this in mind, it does make sense why it bheave like that.

quoted

In the allock skb function we check if the len is 16 and we fix it to a
len of 15.

s/allock/alloc/

quoted

It the read/write function interest to extract the real asked
data. The tagger handler will always copy the fully 16byte with a READ
command. This is useful for some big regs like the fdb reg that are
more than 4byte of data. This permits to introduce a bulk function that
will send and request the entire entry in one go.
Write function is changed and it does now require to pass the pointer to
val to also handle array val.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
  drivers/net/dsa/qca8k.c | 56 ++++++++++++++++++++++++++++++-----------
  1 file changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 2a43fb9aeef2..0183ce2d5b74 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c

@@ -219,7 +219,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
  	if (cmd == MDIO_READ) {
  		mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
-		/* Get the rest of the 12 byte of data */
+		/* Get the rest of the 12 byte of data.
+		 * The read/write function will extract the requested data.
+		 */
  		if (len > QCA_HDR_MGMT_DATA1_LEN)
  			memcpy(mgmt_hdr_data->data + 1, skb->data,
  			       QCA_HDR_MGMT_DATA2_LEN);

@@ -229,16 +231,30 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
  }
  static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
-					       int seq_num, int priority)
+					       int seq_num, int priority, int len)

unsigned int len

quoted

  {
  	struct mgmt_ethhdr *mgmt_ethhdr;
  	struct sk_buff *skb;
+	int real_len;

Likewise.

quoted

+	u32 *data2;
  	u16 hdr;
  	skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
  	if (!skb)
  		return NULL;
+	/* Max value for len reg is 15 (0xf) but the switch actually return 16 byte
+	 * Actually for some reason the steps are:
+	 * 0: nothing
+	 * 1-4: first 4 byte
+	 * 5-6: first 12 byte
+	 * 7-15: all 16 byte
+	 */
+	if (len == 16)
+		real_len = 15;
+	else
+		real_len = len;
+
  	skb_reset_mac_header(skb);
  	skb_set_network_header(skb, skb->len);

@@ -253,7 +269,7 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
  	mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
  	mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
-	mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
+	mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, real_len);
  	mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
  	mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
  					   QCA_HDR_MGMT_CHECK_CODE_VAL);

@@ -263,19 +279,22 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
  	mgmt_ethhdr->hdr = htons(hdr);
-	skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
+	data2 = skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
+	if (cmd == MDIO_WRITE && len > QCA_HDR_MGMT_DATA1_LEN)
+		memcpy(data2, val + 1, len - QCA_HDR_MGMT_DATA1_LEN);
  	return skb;
  }
-static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
+static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
  {
  	struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
  	struct sk_buff *skb;
  	bool ack;
  	int ret;
-	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200,
+				      QCA8K_ETHERNET_MDIO_PRIORITY, len);
  	if (!skb)
  		return -ENOMEM;

@@ -297,6 +316,9 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
  					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
  	*val = mgmt_hdr_data->data[0];
+	if (len > QCA_HDR_MGMT_DATA1_LEN)
+		memcpy(val + 1, mgmt_hdr_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
+
  	ack = mgmt_hdr_data->ack;
  	mutex_unlock(&mgmt_hdr_data->mutex);

@@ -310,14 +332,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
  	return 0;
  }
-static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
+static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
  {
  	struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
  	struct sk_buff *skb;
  	bool ack;
  	int ret;
-	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val, 200,
+				      QCA8K_ETHERNET_MDIO_PRIORITY, len);
  	if (!skb)
  		return -ENOMEM;

@@ -357,14 +380,14 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
  	u32 val = 0;
  	int ret;
-	ret = qca8k_read_eth(priv, reg, &val);
+	ret = qca8k_read_eth(priv, reg, &val, 4);

sizeof(val) instead of 4.

quoted

  	if (ret)
  		return ret;
  	val &= ~mask;
  	val |= write_val;
-	return qca8k_write_eth(priv, reg, val);
+	return qca8k_write_eth(priv, reg, &val, 4);

Likewise

quoted

  }
  static int

@@ -376,7 +399,7 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
  	u16 r1, r2, page;
  	int ret;
-	if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
+	if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val, 4))

Likewise and everywhere below as well.
-- 
Florian

-- 
	Ansuel