[PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Hauke Mehrtens]

This was tested on a Buffalo WSR-2533DHP2. This is a board using a 
Mediatek MT7622 SoC and its 2.5G Ethernet MAC connected over a 2.5G 
HSGMII link to a RTL8367S switch providing 5 1G ports.
This is the only board I have using this switch.

With the DSA_TAG_PROTO_RTL8_4 tag format the TCP checksum for all TCP 
send packets is wrong. It is set to 0x83c6. The mac driver probably 
should do some TCP checksum offload, but it does not work. 

When I used the DSA_TAG_PROTO_RTL8_4T tag format the send packets are 
ok, but it looks like the system does TCP Large receive offload, but 
does not update the TCP checksum correctly. I see that multiple received 
TCP packets are combined into one (using tcpdump on switch port on 
device). The switch tag is also correctly removed. tcpdump complains
that the checksum is wrong, it was updated somewhere, but it is wrong.

Does anyone know what could be wrong here and how to fix this?

This uses the rtl8367s-sgmii.bin firmware file. I extracted it from a 
GPL driver source code with a GPL notice on top. I do not have the 
source code of this firmware. You can download it here:
https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.bin
Here are some information about the source:
https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.txt

This file does not look like intentional GPL. It would be nice if 
Realtek could send this file or a similar version to the linux-firmware 
repository under a license which allows redistribution. I do not have 
any contact at Realtek, if someone has a contact there it would be nice 
if we can help me on this topic.

Hauke Mehrtens (4):
  net: dsa: realtek: rtl8365mb: Fix interface type mask
  net: dsa: realtek: rtl8365mb: Get chip option
  net: dsa: realtek: rtl8365mb: Add setting MTU
  net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

 drivers/net/dsa/realtek/rtl8365mb.c | 444 ++++++++++++++++++++++++++--
 1 file changed, 413 insertions(+), 31 deletions(-)

-- 
2.30.2

[PATCH 1/4] net: dsa: realtek: rtl8365mb: Fix interface type mask

[Hauke Mehrtens]

The mask should be shifted like the offset.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 3d70e8a77ecf..2cb722a9e096 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -237,7 +237,7 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
 		 (_extint) == 2 ? RTL8365MB_DIGITAL_INTERFACE_SELECT_REG1 : \
 		 0x0)
 #define   RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(_extint) \
-		(0xF << (((_extint) % 2)))
+		(0xF << (((_extint) % 2) * 4))
 #define   RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(_extint) \
 		(((_extint) % 2) * 4)
 
-- 
2.30.2

[PATCH 2/4] net: dsa: realtek: rtl8365mb: Get chip option

[Hauke Mehrtens]

Read the option register in addition to the other registers to identify
the chip. The SGMII initialization is different for the different chip
options.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 43 +++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 2cb722a9e096..be64cfdeccc7 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -127,6 +127,9 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
 
 #define RTL8365MB_CHIP_VER_REG		0x1301
 
+#define RTL8365MB_CHIP_OPTION_REG	0x13C1
+
+#define RTL8365MB_MAGIC_OPT_REG		0x13C0
 #define RTL8365MB_MAGIC_REG		0x13C2
 #define   RTL8365MB_MAGIC_VALUE		0x0249
 
@@ -579,6 +582,7 @@ struct rtl8365mb {
 	int irq;
 	u32 chip_id;
 	u32 chip_ver;
+	u32 chip_option;
 	u32 port_mask;
 	u32 learn_limit_max;
 	struct rtl8365mb_cpu cpu;
@@ -1959,7 +1963,7 @@ static void rtl8365mb_teardown(struct dsa_switch *ds)
 	rtl8365mb_irq_teardown(priv);
 }
 
-static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
+static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver, u32 *option)
 {
 	int ret;
 
@@ -1983,6 +1987,19 @@ static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
 	if (ret)
 		return ret;
 
+	ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, RTL8365MB_MAGIC_VALUE);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(map, RTL8365MB_CHIP_OPTION_REG, option);
+	if (ret)
+		return ret;
+
+	/* Reset magic register */
+	ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, 0);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -1991,9 +2008,10 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 	struct rtl8365mb *mb = priv->chip_data;
 	u32 chip_id;
 	u32 chip_ver;
+	u32 chip_option;
 	int ret;
 
-	ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver);
+	ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver, &chip_option);
 	if (ret) {
 		dev_err(priv->dev, "failed to read chip id and version: %d\n",
 			ret);
@@ -2005,22 +2023,22 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 		switch (chip_ver) {
 		case RTL8365MB_CHIP_VER_8365MB_VC:
 			dev_info(priv->dev,
-				 "found an RTL8365MB-VC switch (ver=0x%04x)\n",
-				 chip_ver);
+				 "found an RTL8365MB-VC switch (ver=0x%04x, opt=0x%04x)\n",
+				 chip_ver, chip_option);
 			break;
 		case RTL8365MB_CHIP_VER_8367RB:
 			dev_info(priv->dev,
-				 "found an RTL8367RB-VB switch (ver=0x%04x)\n",
-				 chip_ver);
+				 "found an RTL8367RB-VB switch (ver=0x%04x, opt=0x%04x)\n",
+				 chip_ver, chip_option);
 			break;
 		case RTL8365MB_CHIP_VER_8367S:
 			dev_info(priv->dev,
-				 "found an RTL8367S switch (ver=0x%04x)\n",
-				 chip_ver);
+				 "found an RTL8367S switch (ver=0x%04x, opt=0x%04x)\n",
+				 chip_ver, chip_option);
 			break;
 		default:
-			dev_err(priv->dev, "unrecognized switch version (ver=0x%04x)",
-				chip_ver);
+			dev_err(priv->dev, "unrecognized switch version (ver=0x%04x, opt=0x%04x)",
+				chip_ver, chip_option);
 			return -ENODEV;
 		}
 
@@ -2029,6 +2047,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 		mb->priv = priv;
 		mb->chip_id = chip_id;
 		mb->chip_ver = chip_ver;
+		mb->chip_option = chip_option;
 		mb->port_mask = GENMASK(priv->num_ports - 1, 0);
 		mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
 		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
@@ -2043,8 +2062,8 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 		break;
 	default:
 		dev_err(priv->dev,
-			"found an unknown Realtek switch (id=0x%04x, ver=0x%04x)\n",
-			chip_id, chip_ver);
+			"found an unknown Realtek switch (id=0x%04x, ver=0x%04x, opt=0x%04x)\n",
+			chip_id, chip_ver, chip_option);
 		return -ENODEV;
 	}
 
-- 
2.30.2

[PATCH 3/4] net: dsa: realtek: rtl8365mb: Add setting MTU

[Hauke Mehrtens]

The switch does not support per port MTU setting, but only a MRU
setting. Implement this by setting the MTU on the CPU port.

Without this patch the MRU was always set to 1536, not it is set by the
DSA subsystem and the user scan change it.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index be64cfdeccc7..f9b690251155 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port,
 	return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask);
 }
 
+static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port,
+				     int new_mtu)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct realtek_priv *priv = ds->priv;
+	int length;
+
+	/* When a new MTU is set, DSA always set the CPU port's MTU to the
+	 * largest MTU of the slave ports. Because the switch only has a global
+	 * RX length register, only allowing CPU port here is enough.
+	 */
+	if (!dsa_is_cpu_port(ds, port))
+		return 0;
+
+	length = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+	length += dp->tag_ops->needed_headroom;
+	length += dp->tag_ops->needed_tailroom;
+
+	if (length > RTL8365MB_CFG0_MAX_LEN_MASK)
+		return -EINVAL;
+
+	return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
+				  RTL8365MB_CFG0_MAX_LEN_MASK,
+				  FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK,
+					     length));
+}
+
+static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port)
+{
+	return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8;
+}
+
 static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port,
 				      u32 offset, u32 length, u64 *mibvalue)
 {
@@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		p->index = i;
 	}
 
-	/* Set maximum packet length to 1536 bytes */
-	ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
-				 RTL8365MB_CFG0_MAX_LEN_MASK,
-				 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536));
-	if (ret)
-		goto out_teardown_irq;
-
 	if (priv->setup_interface) {
 		ret = priv->setup_interface(ds);
 		if (ret) {
@@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 	.phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
 	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
 	.port_stp_state_set = rtl8365mb_port_stp_state_set,
+	.port_change_mtu = rtl8365mb_port_change_mtu,
+	.port_max_mtu = rtl8365mb_port_max_mtu,
 	.get_strings = rtl8365mb_get_strings,
 	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
 	.get_sset_count = rtl8365mb_get_sset_count,
@@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.phy_read = rtl8365mb_dsa_phy_read,
 	.phy_write = rtl8365mb_dsa_phy_write,
 	.port_stp_state_set = rtl8365mb_port_stp_state_set,
+	.port_change_mtu = rtl8365mb_port_change_mtu,
+	.port_max_mtu = rtl8365mb_port_max_mtu,
 	.get_strings = rtl8365mb_get_strings,
 	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
 	.get_sset_count = rtl8365mb_get_sset_count,
-- 
2.30.2

[PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Hauke Mehrtens]

This adds support for SGMII and HSGMII on RTL8367S switches.
HSGMII is configured using the 2500BASEX mode.
This is baed on the rtl8367c driver found in OpenWrt.

For (H)SGMII mode we have to load a firmware into some memory which gets
executed on the integrated 8051. The firmware binary was added as a
array into the driver with a GPL license notice on top.

This was tested on RTL8367S (ver=0x00a0, opt=0x0001).

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 356 +++++++++++++++++++++++++++-
 1 file changed, 345 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index f9b690251155..5504a34fffeb 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
  * Copyright (C) 2021 Michael Rasmussen <mir@bang-olufsen.dk>
+ * Copyright (C) 2022 Hauke Mehrtens <hauke@hauke-m.de>
  *
  * The RTL8365MB-VC is a 4+1 port 10/100/1000M switch controller. It includes 4
  * integrated PHYs for the user facing ports, and an extension interface which
@@ -98,6 +99,7 @@
 #include <linux/of_irq.h>
 #include <linux/regmap.h>
 #include <linux/if_bridge.h>
+#include <linux/firmware.h>
 
 #include "realtek.h"
 
@@ -135,6 +137,7 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
 
 /* Chip reset register */
 #define RTL8365MB_CHIP_RESET_REG	0x1322
+#define RTL8365MB_CHIP_RESET_DW8051	BIT(4)
 #define RTL8365MB_CHIP_RESET_SW_MASK	0x0002
 #define RTL8365MB_CHIP_RESET_HW_MASK	0x0001
 
@@ -278,6 +281,29 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
 #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK		0x0004
 #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK		0x0003
 
+#define RTL8365MB_SDS_MISC			0x1d11
+#define  RTL8365MB_CFG_SGMII_RXFC		0x4000
+#define  RTL8365MB_CFG_SGMII_TXFC		0x2000
+#define  RTL8365MB_INB_ARB			0x1000
+#define  RTL8365MB_CFG_MAC8_SEL_HSGMII		0x0800
+#define  RTL8365MB_CFG_SGMII_FDUP		0x0400
+#define  RTL8365MB_CFG_SGMII_LINK		0x0200
+#define  RTL8365MB_CFG_SGMII_SPD		0x0180
+#define  RTL8365MB_CFG_MAC8_SEL_SGMII		0x0040
+#define  RTL8365MB_CFG_INB_SEL			0x0038
+#define  RTL8365MB_CFG_SDS_MODE_18C		0x0007
+
+#define RTL8365MB_SDS_INDACS_CMD		0x6600
+#define RTL8365MB_SDS_INDACS_ADR		0x6601
+#define RTL8365MB_SDS_INDACS_DATA		0x6602
+
+#define RTL8365MB_MISC_CFG0			0x130c
+#define  RTL8365MB_MISC_CFG0_DW8051_EN		BIT(5)
+
+#define RTL8365MB_DW8051_RDY			0x1336
+#define  RTL8365MB_DW8051_RDY_IROM_MSB		BIT(2)
+#define  RTL8365MB_DW8051_RDY_ACS_IROM_EN	BIT(1)
+
 /* CPU port mask register - controls which ports are treated as CPU ports */
 #define RTL8365MB_CPU_PORT_MASK_REG	0x1219
 #define   RTL8365MB_CPU_PORT_MASK_MASK	0x07FF
@@ -296,6 +322,8 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
 #define RTL8365MB_CFG0_MAX_LEN_REG	0x088C
 #define   RTL8365MB_CFG0_MAX_LEN_MASK	0x3FFF
 
+#define RTL8365MB_BYPASS_LINE_RATE		0x03f7
+
 /* Port learning limit registers */
 #define RTL8365MB_LUT_PORT_LEARN_LIMIT_BASE		0x0A20
 #define RTL8365MB_LUT_PORT_LEARN_LIMIT_REG(_physport) \
@@ -493,6 +521,39 @@ static const struct rtl8365mb_jam_tbl_entry rtl8365mb_init_jam_common[] = {
 	{ 0x1D32, 0x0002 },
 };
 
+struct rtl8365mb_sds_init {
+	unsigned int data;
+	unsigned int addr;
+};
+
+static const struct rtl8365mb_sds_init redData[] = {
+	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x21A2, 0x0482}, {0x6960, 0x0483},
+	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
+};
+
+static const struct rtl8365mb_sds_init redDataSB[] = {
+	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x31A2, 0x0482}, {0x6960, 0x0483},
+	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
+};
+
+static const struct rtl8365mb_sds_init redData1_5_6[] = {
+	{0x82F1, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
+	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
+	{0x83F2, 0x002E}
+};
+
+static const struct rtl8365mb_sds_init redData8_9[] = {
+	{0x82F1, 0x0500}, {0xF995, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
+	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
+	{0x83F2, 0x002E}
+};
+
+static const struct rtl8365mb_sds_init redDataHB[] = {
+	{0x82F0, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x7960, 0x0503},
+	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
+	{0x83F2, 0x002E}
+};
+
 enum rtl8365mb_stp_state {
 	RTL8365MB_STP_STATE_DISABLED = 0,
 	RTL8365MB_STP_STATE_BLOCKING = 1,
@@ -801,6 +862,232 @@ rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
 	return DSA_TAG_PROTO_RTL8_4;
 }
 
+static int rtl8365mb_sds_indacs_write(struct realtek_priv *priv, unsigned int addr,
+				      unsigned int data)
+{
+	int ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, data);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);
+	if (ret)
+		return ret;
+
+	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);
+}
+
+static int rtl8365mb_sds_indacs_read(struct realtek_priv *priv, unsigned int addr,
+				     unsigned int *data)
+{
+	int ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);
+	if (ret)
+		return ret;
+
+	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, *data);
+}
+
+static int rtl8365mb_ext_init_sgmii_fw(struct realtek_priv *priv)
+{
+	struct device *dev = priv->dev;
+	const struct firmware *fw;
+	int ret;
+	int i;
+
+	ret = request_firmware(&fw, "rtl_switch/rtl8367s-sgmii.bin", dev);
+	if (ret) {
+		dev_err(dev, "failed to load firmware rtl_switch/rtl8367s-sgmii.bin, error: %i\n",
+			ret);
+		return ret;
+	}
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
+				 RTL8365MB_CHIP_RESET_DW8051,
+				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 1));
+	if (ret)
+		goto release_fw;
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
+				 RTL8365MB_MISC_CFG0_DW8051_EN,
+				 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
+	if (ret)
+		goto release_fw;
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
+				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
+				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 1));
+	if (ret)
+		goto release_fw;
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
+				 RTL8365MB_DW8051_RDY_IROM_MSB,
+				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));
+	if (ret)
+		goto release_fw;
+
+	for (i = 0; i < fw->size; i++) {
+		ret = regmap_write(priv->map, 0xE000 + i, fw->data[i]);
+		if (ret)
+			goto release_fw;
+	}
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
+				 RTL8365MB_DW8051_RDY_IROM_MSB,
+				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));
+	if (ret)
+		goto release_fw;
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
+				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
+				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 0));
+	if (ret)
+		goto release_fw;
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
+				 RTL8365MB_CHIP_RESET_DW8051,
+				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 0));
+
+release_fw:
+	release_firmware(fw);
+	return ret;
+}
+
+static int rtl8365mb_ext_init_sgmii(struct realtek_priv *priv, int port, phy_interface_t interface)
+{
+	struct rtl8365mb *mb;
+	int interface_mode;
+	int sds_mode;
+	const struct rtl8365mb_sds_init *sds_init;
+	size_t sds_init_len;
+	int ext_int;
+	int ret;
+	int i;
+	int val;
+	int mask;
+
+	mb = priv->chip_data;
+
+	if (mb->chip_id != RTL8365MB_CHIP_ID_8365MB_VC)
+		return -EINVAL;
+
+	ext_int = rtl8365mb_extint_port_map[port];
+	if (ext_int != 1)
+		return -EINVAL;
+
+	if (interface == PHY_INTERFACE_MODE_SGMII) {
+		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_SGMII, 1);
+		interface_mode = RTL8365MB_EXT_PORT_MODE_SGMII;
+
+		if (mb->chip_option == 0) {
+			sds_init = redData;
+			sds_init_len = ARRAY_SIZE(redData);
+		} else {
+			sds_init = redDataSB;
+			sds_init_len = ARRAY_SIZE(redDataSB);
+		}
+	} else if (interface == PHY_INTERFACE_MODE_2500BASEX) {
+		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_HSGMII, 1);
+		interface_mode = RTL8365MB_EXT_PORT_MODE_HSGMII;
+
+		if (mb->chip_option == 0) {
+			switch (mb->chip_ver & 0x00F0) {
+			case 0x0010:
+			case 0x0050:
+			case 0x0060:
+				sds_init = redData1_5_6;
+				sds_init_len = ARRAY_SIZE(redData1_5_6);
+				break;
+			case 0x0080:
+			case 0x0090:
+				sds_init = redData8_9;
+				sds_init_len = ARRAY_SIZE(redData8_9);
+				break;
+			default:
+				return -EINVAL;
+			}
+		} else {
+			sds_init = redDataHB;
+			sds_init_len = ARRAY_SIZE(redDataHB);
+		}
+	} else {
+		return -EINVAL;
+	}
+
+	for (i = 0; i < sds_init_len; i++) {
+		ret = rtl8365mb_sds_indacs_write(priv, sds_init[i].addr, sds_init[i].data);
+		if (ret)
+			return ret;
+	}
+
+	mask = RTL8365MB_CFG_MAC8_SEL_SGMII | RTL8365MB_CFG_MAC8_SEL_HSGMII;
+	ret = regmap_update_bits(priv->map,
+				 RTL8365MB_SDS_MISC,
+				 mask,
+				 sds_mode);
+	if (ret)
+		return ret;
+
+	mask = RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(ext_int);
+	val = interface_mode << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(ext_int);
+	ret = regmap_update_bits(priv->map,
+				 RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(ext_int),
+				 mask,
+				 val);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_BYPASS_LINE_RATE, 0x0);
+	if (ret)
+		return ret;
+
+	/* Serdes not reset */
+	ret = rtl8365mb_sds_indacs_write(priv, 0x0003, 0x7106);
+	if (ret)
+		return ret;
+
+	return rtl8365mb_ext_init_sgmii_fw(priv);
+}
+
+static int rtl8365mb_ext_sgmii_nway(struct realtek_priv *priv, bool state)
+{
+	u32 running;
+	u32 regValue;
+	int ret;
+
+	ret = regmap_read(priv->map, RTL8365MB_MISC_CFG0, &running);
+	if (running & RTL8365MB_MISC_CFG0_DW8051_EN) {
+		ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
+					 RTL8365MB_MISC_CFG0_DW8051_EN,
+					 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 0));
+		if (ret)
+			return ret;
+	}
+
+	ret = rtl8365mb_sds_indacs_read(priv, 0x0002, &regValue);
+	if (ret)
+		return ret;
+
+	if (state)
+		regValue |= 0x0200;
+	else
+		regValue &= ~0x0200;
+	regValue |= 0x0100;
+
+	ret = rtl8365mb_sds_indacs_write(priv, 0x0002, regValue);
+	if (ret)
+		return ret;
+	return regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
+				  RTL8365MB_MISC_CFG0_DW8051_EN,
+				  FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
+}
+
 static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
 				      phy_interface_t interface)
 {
@@ -886,6 +1173,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
 }
 
 static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
+					  phy_interface_t interface,
 					  bool link, int speed, int duplex,
 					  bool tx_pause, bool rx_pause)
 {
@@ -911,7 +1199,7 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
 		r_rx_pause = rx_pause ? 1 : 0;
 		r_tx_pause = tx_pause ? 1 : 0;
 
-		if (speed == SPEED_1000) {
+		if (speed == SPEED_1000 || speed == SPEED_2500) {
 			r_speed = RTL8365MB_PORT_SPEED_1000M;
 		} else if (speed == SPEED_100) {
 			r_speed = RTL8365MB_PORT_SPEED_100M;
@@ -941,6 +1229,25 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
 		r_duplex = 0;
 	}
 
+	if (interface == PHY_INTERFACE_MODE_SGMII ||
+	    interface == PHY_INTERFACE_MODE_2500BASEX) {
+		val = FIELD_PREP(RTL8365MB_CFG_SGMII_FDUP, r_duplex) |
+		      FIELD_PREP(RTL8365MB_CFG_SGMII_SPD, r_speed) |
+		      FIELD_PREP(RTL8365MB_CFG_SGMII_LINK, r_link) |
+		      FIELD_PREP(RTL8365MB_CFG_SGMII_TXFC, r_tx_pause) |
+		      FIELD_PREP(RTL8365MB_CFG_SGMII_RXFC, r_rx_pause);
+		ret = regmap_update_bits(priv->map,
+					 RTL8365MB_SDS_MISC,
+					 RTL8365MB_CFG_SGMII_FDUP |
+					 RTL8365MB_CFG_SGMII_SPD |
+					 RTL8365MB_CFG_SGMII_LINK |
+					 RTL8365MB_CFG_SGMII_TXFC |
+					 RTL8365MB_CFG_SGMII_RXFC,
+					 val);
+		if (ret)
+			return ret;
+	}
+
 	val = FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK, 1) |
 	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK,
 			 r_tx_pause) |
@@ -972,10 +1279,15 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
 	     interface == PHY_INTERFACE_MODE_GMII))
 		/* Internal PHY */
 		return true;
-	else if ((ext_int >= 1) &&
-		 phy_interface_mode_is_rgmii(interface))
+	else if ((ext_int == 1) &&
+		 (phy_interface_mode_is_rgmii(interface) ||
+		  interface == PHY_INTERFACE_MODE_SGMII ||
+		  interface == PHY_INTERFACE_MODE_2500BASEX))
 		/* Extension MAC */
 		return true;
+	else if ((ext_int >= 2) &&
+		 phy_interface_mode_is_rgmii(interface))
+		return true;
 
 	return false;
 }
@@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
 static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
 				       struct phylink_config *config)
 {
-	if (dsa_is_user_port(ds, port))
+	int ext_int = rtl8365mb_extint_port_map[port];
+
+	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
+				   MAC_10 | MAC_100 | MAC_1000FD;
+
+	if (dsa_is_user_port(ds, port)) {
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
-	else if (dsa_is_cpu_port(ds, port))
+	} else if (dsa_is_cpu_port(ds, port)) {
+		if (ext_int == 1) {
+			__set_bit(PHY_INTERFACE_MODE_SGMII,
+				  config->supported_interfaces);
+			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+				  config->supported_interfaces);
+			config->mac_capabilities |= MAC_2500FD;
+		}
 		phy_interface_set_rgmii(config->supported_interfaces);
+	}
 
-	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
-				   MAC_10 | MAC_100 | MAC_1000FD;
 }
 
 static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
@@ -1020,6 +1343,10 @@ static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
 				"failed to configure RGMII mode on port %d: %d\n",
 				port, ret);
 		return;
+	} else if (state->interface == PHY_INTERFACE_MODE_SGMII ||
+		   state->interface == PHY_INTERFACE_MODE_2500BASEX) {
+		rtl8365mb_ext_init_sgmii(priv, port, state->interface);
+		rtl8365mb_ext_sgmii_nway(priv, false);
 	}
 
 	/* TODO: Implement MII and RMII modes, which the RTL8365MB-VC also
@@ -1040,8 +1367,11 @@ static void rtl8365mb_phylink_mac_link_down(struct dsa_switch *ds, int port,
 	p = &mb->ports[port];
 	cancel_delayed_work_sync(&p->mib_work);
 
-	if (phy_interface_mode_is_rgmii(interface)) {
-		ret = rtl8365mb_ext_config_forcemode(priv, port, false, 0, 0,
+	if (phy_interface_mode_is_rgmii(interface) ||
+	    interface == PHY_INTERFACE_MODE_SGMII ||
+	    interface == PHY_INTERFACE_MODE_2500BASEX) {
+		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
+						     false, 0, 0,
 						     false, false);
 		if (ret)
 			dev_err(priv->dev,
@@ -1068,8 +1398,11 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
 	p = &mb->ports[port];
 	schedule_delayed_work(&p->mib_work, 0);
 
-	if (phy_interface_mode_is_rgmii(interface)) {
-		ret = rtl8365mb_ext_config_forcemode(priv, port, true, speed,
+	if (phy_interface_mode_is_rgmii(interface) ||
+	    interface == PHY_INTERFACE_MODE_SGMII ||
+	    interface == PHY_INTERFACE_MODE_2500BASEX) {
+		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
+						     true, speed,
 						     duplex, tx_pause,
 						     rx_pause);
 		if (ret)
@@ -2156,6 +2489,7 @@ const struct realtek_variant rtl8365mb_variant = {
 };
 EXPORT_SYMBOL_GPL(rtl8365mb_variant);
 
+MODULE_FIRMWARE("rtl_switch/rtl8367s-sgmii.bin");
 MODULE_AUTHOR("Alvin Šipraga <alsi@bang-olufsen.dk>");
 MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch");
 MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Luiz Angelo Daros de Luca]

> This was tested on a Buffalo WSR-2533DHP2. This is a board using a
> Mediatek MT7622 SoC and its 2.5G Ethernet MAC connected over a 2.5G
> HSGMII link to a RTL8367S switch providing 5 1G ports.
> This is the only board I have using this switch.
>
> With the DSA_TAG_PROTO_RTL8_4 tag format the TCP checksum for all TCP
> send packets is wrong. It is set to 0x83c6. The mac driver probably
> should do some TCP checksum offload, but it does not work.

0x83c6 might be yout tcp pseudo ip header sum.

> When I used the DSA_TAG_PROTO_RTL8_4T tag format the send packets are
> ok, but it looks like the system does TCP Large receive offload, but
> does not update the TCP checksum correctly. I see that multiple received
> TCP packets are combined into one (using tcpdump on switch port on
> device). The switch tag is also correctly removed. tcpdump complains
> that the checksum is wrong, it was updated somewhere, but it is wrong.
>
> Does anyone know what could be wrong here and how to fix this?

The good news, it is a known problem:

https://patchwork.kernel.org/project/netdevbpf/patch/20220411230305.28951-1-luizluca@gmail.com/
(there are some interesting discussions)
https://patchwork.kernel.org/project/netdevbpf/patch/20220416052737.25509-1-luizluca@gmail.com/
(merged)

The bad news, there is no way to enable checksum offload with
mediatek+realtek. You'll need to disable almost any type of offload.
For any tag before the IP header, if your driver/HW does not support a
way to set where the IP header starts and the offload HW does not
understand the tag protocol, the offload HW will keep the pseudo ip
header sum. And for tags after the payload, the offload HW will blend
the tag with the payload, generating bad checksums when the switch
removes the tag.

You can do that from userland, using ethtool on the master port before
the bridge is up, or patching the driver. You can try this patch
(written for MT7620 but it is easy to adapt it to upstream
mtk_eth_soc.c).
https://github.com/luizluca/openwrt/commit/d799bd363f902bf3b9c595972a1b9280a0b61dca
. I never submitted that upstream because I don't have the HW to test
it (Arinç tested a modified version in an MT7621) and I didn't
investigate how much those extra ifs in ndo_features_check will cost
in performance when the driver does support the tag (using a mediatek
switch).

And the DSA_TAG_PROTO_RTL8_4T already paid off. It was added exactly
as a way to test checksum errors. Probably no offload will work for
tags that are after the payload if the offload HW does not already
know that tag (e.g. same vendors). DSA_TAG_PROTO_RTL8_4T works because
it calculates the checksum in software before the tag is added.
However, during my tests, I never tested TCP Large receive offload.

> This uses the rtl8367s-sgmii.bin firmware file. I extracted it from a
> GPL driver source code with a GPL notice on top. I do not have the
> source code of this firmware. You can download it here:
> https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.bin
> Here are some information about the source:
> https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.txt
>
> This file does not look like intentional GPL. It would be nice if
> Realtek could send this file or a similar version to the linux-firmware
> repository under a license which allows redistribution. I do not have
> any contact at Realtek, if someone has a contact there it would be nice
> if we can help me on this topic.
>
> Hauke Mehrtens (4):
>   net: dsa: realtek: rtl8365mb: Fix interface type mask
>   net: dsa: realtek: rtl8365mb: Get chip option
>   net: dsa: realtek: rtl8365mb: Add setting MTU
>   net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
>
>  drivers/net/dsa/realtek/rtl8365mb.c | 444 ++++++++++++++++++++++++++--
>  1 file changed, 413 insertions(+), 31 deletions(-)
>
> --
> 2.30.2
>

Re: [PATCH 3/4] net: dsa: realtek: rtl8365mb: Add setting MTU

[Luiz Angelo Daros de Luca]

> The switch does not support per port MTU setting, but only a MRU
> setting. Implement this by setting the MTU on the CPU port.
>
> Without this patch the MRU was always set to 1536, not it is set by the
> DSA subsystem and the user scan change it.
>
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index be64cfdeccc7..f9b690251155 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port,
>         return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask);
>  }
>
> +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port,
> +                                    int new_mtu)
> +{
> +       struct dsa_port *dp = dsa_to_port(ds, port);
> +       struct realtek_priv *priv = ds->priv;
> +       int length;
> +
> +       /* When a new MTU is set, DSA always set the CPU port's MTU to the
> +        * largest MTU of the slave ports. Because the switch only has a global
> +        * RX length register, only allowing CPU port here is enough.
> +        */
> +       if (!dsa_is_cpu_port(ds, port))
> +               return 0;
> +
> +       length = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> +       length += dp->tag_ops->needed_headroom;
> +       length += dp->tag_ops->needed_tailroom;

Isn't it better to keep that within the driver? No matter the tag
position, it will be either 4 (RTL8365MB_CPU_FORMAT_4BYTES) or 8
(RTL8365MB_CPU_FORMAT_8BYTES) bytes. You can retrieve that from
priv->chip_data->cpu->format, but the driver will probably never
support RTL8365MB_CPU_FORMAT_4BYTES. Until someone does implement the
4-bytes tag (for some mysterious reason), I believe we could simply
use a constant here (using a proper new macro).

> +
> +       if (length > RTL8365MB_CFG0_MAX_LEN_MASK)
> +               return -EINVAL;
> +
> +       return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> +                                 RTL8365MB_CFG0_MAX_LEN_MASK,
> +                                 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK,
> +                                            length));
> +}
> +
> +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> +       return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8;

What is this magic 8? RTL8_4_TAG_LEN?

> +}
> +
>  static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port,
>                                       u32 offset, u32 length, u64 *mibvalue)
>  {
> @@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>                 p->index = i;
>         }
>
> -       /* Set maximum packet length to 1536 bytes */
> -       ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> -                                RTL8365MB_CFG0_MAX_LEN_MASK,
> -                                FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536));
> -       if (ret)
> -               goto out_teardown_irq;
> -
>         if (priv->setup_interface) {
>                 ret = priv->setup_interface(ds);
>                 if (ret) {
> @@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>         .phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
>         .phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
>         .port_stp_state_set = rtl8365mb_port_stp_state_set,
> +       .port_change_mtu = rtl8365mb_port_change_mtu,
> +       .port_max_mtu = rtl8365mb_port_max_mtu,
>         .get_strings = rtl8365mb_get_strings,
>         .get_ethtool_stats = rtl8365mb_get_ethtool_stats,
>         .get_sset_count = rtl8365mb_get_sset_count,
> @@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
>         .phy_read = rtl8365mb_dsa_phy_read,
>         .phy_write = rtl8365mb_dsa_phy_write,
>         .port_stp_state_set = rtl8365mb_port_stp_state_set,
> +       .port_change_mtu = rtl8365mb_port_change_mtu,
> +       .port_max_mtu = rtl8365mb_port_max_mtu,
>         .get_strings = rtl8365mb_get_strings,
>         .get_ethtool_stats = rtl8365mb_get_ethtool_stats,
>         .get_sset_count = rtl8365mb_get_sset_count,
> --
> 2.30.2
>

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Luiz Angelo Daros de Luca]

> > Hauke Mehrtens (4):
> >   net: dsa: realtek: rtl8365mb: Fix interface type mask
> >   net: dsa: realtek: rtl8365mb: Get chip option
> >   net: dsa: realtek: rtl8365mb: Add setting MTU
> >   net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

I didn't get these two, although patchwork got them:

  net: dsa: realtek: rtl8365mb: Get chip option
  net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

Re: [PATCH 2/4] net: dsa: realtek: rtl8365mb: Get chip option

[Luiz Angelo Daros de Luca]

> Read the option register in addition to the other registers to identify
> the chip. The SGMII initialization is different for the different chip
> options.

Is it possible to have two different chip models that share the same
chip ip and version but differ only in the option?
If that is true and the driver still wishes to print the correct model
name, we should keep track of each option value for each supported
chip, just like we already do with id/version.
If not, I don't believe we need to print that out during probe because
it would be just a derived property from chip id/model.

Regards,

Luiz

Re: [PATCH 3/4] net: dsa: realtek: rtl8365mb: Add setting MTU

[Andrew Lunn]

> > +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port,
> > +                                    int new_mtu)
> > +{
> > +       struct dsa_port *dp = dsa_to_port(ds, port);
> > +       struct realtek_priv *priv = ds->priv;
> > +       int length;
> > +
> > +       /* When a new MTU is set, DSA always set the CPU port's MTU to the
> > +        * largest MTU of the slave ports. Because the switch only has a global
> > +        * RX length register, only allowing CPU port here is enough.
> > +        */
> > +       if (!dsa_is_cpu_port(ds, port))
> > +               return 0;
> > +
> > +       length = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> > +       length += dp->tag_ops->needed_headroom;
> > +       length += dp->tag_ops->needed_tailroom;
> 
> Isn't it better to keep that within the driver? No matter the tag
> position, it will be either 4 (RTL8365MB_CPU_FORMAT_4BYTES) or 8
> (RTL8365MB_CPU_FORMAT_8BYTES) bytes. You can retrieve that from
> priv->chip_data->cpu->format, but the driver will probably never
> support RTL8365MB_CPU_FORMAT_4BYTES. Until someone does implement the
> 4-bytes tag (for some mysterious reason), I believe we could simply
> use a constant here (using a proper new macro).

Another option is to simply always use the bigger header length. I
doubt there are many people actually using jumbo frames, and do they
really care about 0x3FFF-4, vs 0x3FFF-8?
 
> > +
> > +       if (length > RTL8365MB_CFG0_MAX_LEN_MASK)
> > +               return -EINVAL;
> > +
> > +       return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> > +                                 RTL8365MB_CFG0_MAX_LEN_MASK,
> > +                                 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK,
> > +                                            length));
> > +}
> > +
> > +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port)
> > +{
> > +       return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8;
> 
> What is this magic 8? RTL8_4_TAG_LEN?

There are some DSA headers in include/linux/dsa, probably a new one
should be added with this RTL8_4_TAG_LEN.
 
	Andrew

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Florian Fainelli]

On 5/9/2022 12:38 AM, Luiz Angelo Daros de Luca wrote:
>>> Hauke Mehrtens (4):
>>>    net: dsa: realtek: rtl8365mb: Fix interface type mask
>>>    net: dsa: realtek: rtl8365mb: Get chip option
>>>    net: dsa: realtek: rtl8365mb: Add setting MTU
>>>    net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
> 
> I didn't get these two, although patchwork got them:
> 
>    net: dsa: realtek: rtl8365mb: Get chip option
>    net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

Probably yet another instance of poor interaction between gmail.com and 
vger.kernel.org, I got all of them in my inbox.
-- 
Florian

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Vladimir Oltean]

On Mon, May 09, 2022 at 08:36:19AM -0700, Florian Fainelli wrote:
> On 5/9/2022 12:38 AM, Luiz Angelo Daros de Luca wrote:
> > > > Hauke Mehrtens (4):
> > > >    net: dsa: realtek: rtl8365mb: Fix interface type mask
> > > >    net: dsa: realtek: rtl8365mb: Get chip option
> > > >    net: dsa: realtek: rtl8365mb: Add setting MTU
> > > >    net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
> > 
> > I didn't get these two, although patchwork got them:
> > 
> >    net: dsa: realtek: rtl8365mb: Get chip option
> >    net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
> 
> Probably yet another instance of poor interaction between gmail.com and
> vger.kernel.org, I got all of them in my inbox.
> -- 
> Florian

But you were copied to the emails, Luiz wasn't.
I'm also having trouble receiving emails from the mailing list, I get
them with a huge delay (days).

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Florian Fainelli]

On 5/9/2022 8:40 AM, Vladimir Oltean wrote:
> On Mon, May 09, 2022 at 08:36:19AM -0700, Florian Fainelli wrote:
>> On 5/9/2022 12:38 AM, Luiz Angelo Daros de Luca wrote:
>>>>> Hauke Mehrtens (4):
>>>>>     net: dsa: realtek: rtl8365mb: Fix interface type mask
>>>>>     net: dsa: realtek: rtl8365mb: Get chip option
>>>>>     net: dsa: realtek: rtl8365mb: Add setting MTU
>>>>>     net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
>>>
>>> I didn't get these two, although patchwork got them:
>>>
>>>     net: dsa: realtek: rtl8365mb: Get chip option
>>>     net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
>>
>> Probably yet another instance of poor interaction between gmail.com and
>> vger.kernel.org, I got all of them in my inbox.
>> -- 
>> Florian
> 
> But you were copied to the emails, Luiz wasn't.

Yes, that much is true.

> I'm also having trouble receiving emails from the mailing list, I get
> them with a huge delay (days).

Time to switch to a different toolset maybe: 
https://josefbacik.github.io/kernel/2021/10/18/lei-and-b4.html
-- 
Florian

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Vladimir Oltean]

On Mon, May 09, 2022 at 08:41:38AM -0700, Florian Fainelli wrote:
> On 5/9/2022 8:40 AM, Vladimir Oltean wrote:
> > On Mon, May 09, 2022 at 08:36:19AM -0700, Florian Fainelli wrote:
> > > On 5/9/2022 12:38 AM, Luiz Angelo Daros de Luca wrote:
> > > > > > Hauke Mehrtens (4):
> > > > > >     net: dsa: realtek: rtl8365mb: Fix interface type mask
> > > > > >     net: dsa: realtek: rtl8365mb: Get chip option
> > > > > >     net: dsa: realtek: rtl8365mb: Add setting MTU
> > > > > >     net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
> > > > 
> > > > I didn't get these two, although patchwork got them:
> > > > 
> > > >     net: dsa: realtek: rtl8365mb: Get chip option
> > > >     net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
> > > 
> > > Probably yet another instance of poor interaction between gmail.com and
> > > vger.kernel.org, I got all of them in my inbox.
> > > -- 
> > > Florian
> > 
> > But you were copied to the emails, Luiz wasn't.
> 
> Yes, that much is true.
> 
> > I'm also having trouble receiving emails from the mailing list, I get
> > them with a huge delay (days).
> 
> Time to switch to a different toolset maybe:
> https://josefbacik.github.io/kernel/2021/10/18/lei-and-b4.html
> -- 
> Florian

I guess I'll finally try lei out, thanks.

Re: [PATCH 2/4] net: dsa: realtek: rtl8365mb: Get chip option

[Alvin Šipraga]

On Mon, May 09, 2022 at 12:48:46AM +0200, Hauke Mehrtens wrote:
> Read the option register in addition to the other registers to identify
> the chip. The SGMII initialization is different for the different chip
> options.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 43 +++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 2cb722a9e096..be64cfdeccc7 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -127,6 +127,9 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  
>  #define RTL8365MB_CHIP_VER_REG		0x1301
>  
> +#define RTL8365MB_CHIP_OPTION_REG	0x13C1
> +
> +#define RTL8365MB_MAGIC_OPT_REG		0x13C0

Realtek's driver calls this register PROTECT_ID:

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/files/drivers/net/phy/rtk/rtl8367c/include/rtl8367c_reg.h;h=eb4f48b83e02ce0625d70b337c445c36f071686c;hb=HEAD
18050 #define    RTL8367C_REG_PROTECT_ID    0x13c0

Consider renaming it, or at least be consistent with _OPT and _OPTION. I see
both opt and option throughout the patch.

>  #define RTL8365MB_MAGIC_REG		0x13C2
>  #define   RTL8365MB_MAGIC_VALUE		0x0249

Can you repeat this for MAGIC_OPT_REG? It's OK that it's the same.

>  
> @@ -579,6 +582,7 @@ struct rtl8365mb {
>  	int irq;
>  	u32 chip_id;
>  	u32 chip_ver;
> +	u32 chip_option;

Moreover, do you know what option is supposed to mean here? Likewise the
register map from Realtek calls this CHIP_VER_INTL (internal maybe?):

#define    RTL8367C_REG_CHIP_VER_INTL    0x13c1

>  	u32 port_mask;
>  	u32 learn_limit_max;
>  	struct rtl8365mb_cpu cpu;
> @@ -1959,7 +1963,7 @@ static void rtl8365mb_teardown(struct dsa_switch *ds)
>  	rtl8365mb_irq_teardown(priv);
>  }
>  
> -static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
> +static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver, u32 *option)

Likewise for consistency, consider the name of this function as you add a new
argument to it.

>  {
>  	int ret;
>  
> @@ -1983,6 +1987,19 @@ static int rtl8365mb_get_chip_id_and_ver(struct regmap *map, u32 *id, u32 *ver)
>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, RTL8365MB_MAGIC_VALUE);

Might deserve a comment here too, "the ver2/ver_intl/option/etc. register is
also protected with magic".

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, RTL8365MB_CHIP_OPTION_REG, option);
> +	if (ret)
> +		return ret;
> +
> +	/* Reset magic register */
> +	ret = regmap_write(map, RTL8365MB_MAGIC_OPT_REG, 0);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> @@ -1991,9 +2008,10 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  	struct rtl8365mb *mb = priv->chip_data;
>  	u32 chip_id;
>  	u32 chip_ver;
> +	u32 chip_option;
>  	int ret;
>  
> -	ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver);
> +	ret = rtl8365mb_get_chip_id_and_ver(priv->map, &chip_id, &chip_ver, &chip_option);
>  	if (ret) {
>  		dev_err(priv->dev, "failed to read chip id and version: %d\n",
>  			ret);
> @@ -2005,22 +2023,22 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		switch (chip_ver) {
>  		case RTL8365MB_CHIP_VER_8365MB_VC:
>  			dev_info(priv->dev,
> -				 "found an RTL8365MB-VC switch (ver=0x%04x)\n",
> -				 chip_ver);
> +				 "found an RTL8365MB-VC switch (ver=0x%04x, opt=0x%04x)\n",
> +				 chip_ver, chip_option);
>  			break;
>  		case RTL8365MB_CHIP_VER_8367RB:
>  			dev_info(priv->dev,
> -				 "found an RTL8367RB-VB switch (ver=0x%04x)\n",
> -				 chip_ver);
> +				 "found an RTL8367RB-VB switch (ver=0x%04x, opt=0x%04x)\n",
> +				 chip_ver, chip_option);
>  			break;
>  		case RTL8365MB_CHIP_VER_8367S:
>  			dev_info(priv->dev,
> -				 "found an RTL8367S switch (ver=0x%04x)\n",
> -				 chip_ver);
> +				 "found an RTL8367S switch (ver=0x%04x, opt=0x%04x)\n",
> +				 chip_ver, chip_option);
>  			break;
>  		default:
> -			dev_err(priv->dev, "unrecognized switch version (ver=0x%04x)",
> -				chip_ver);
> +			dev_err(priv->dev, "unrecognized switch version (ver=0x%04x, opt=0x%04x)",
> +				chip_ver, chip_option);
>  			return -ENODEV;
>  		}
>  
> @@ -2029,6 +2047,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		mb->priv = priv;
>  		mb->chip_id = chip_id;
>  		mb->chip_ver = chip_ver;
> +		mb->chip_option = chip_option;
>  		mb->port_mask = GENMASK(priv->num_ports - 1, 0);
>  		mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
>  		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> @@ -2043,8 +2062,8 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		break;
>  	default:
>  		dev_err(priv->dev,
> -			"found an unknown Realtek switch (id=0x%04x, ver=0x%04x)\n",
> -			chip_id, chip_ver);
> +			"found an unknown Realtek switch (id=0x%04x, ver=0x%04x, opt=0x%04x)\n",
> +			chip_id, chip_ver, chip_option);
>  		return -ENODEV;
>  	}
>  
> -- 
> 2.30.2
>

Re: [PATCH 1/4] net: dsa: realtek: rtl8365mb: Fix interface type mask

[Alvin Šipraga]

On Mon, May 09, 2022 at 12:48:45AM +0200, Hauke Mehrtens wrote:
> The mask should be shifted like the offset.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 3d70e8a77ecf..2cb722a9e096 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -237,7 +237,7 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  		 (_extint) == 2 ? RTL8365MB_DIGITAL_INTERFACE_SELECT_REG1 : \
>  		 0x0)
>  #define   RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(_extint) \
> -		(0xF << (((_extint) % 2)))
> +		(0xF << (((_extint) % 2) * 4))
>  #define   RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(_extint) \
>  		(((_extint) % 2) * 4)
>  
> -- 
> 2.30.2
>

Re: [PATCH 3/4] net: dsa: realtek: rtl8365mb: Add setting MTU

[Alvin Šipraga]

On Mon, May 09, 2022 at 12:48:47AM +0200, Hauke Mehrtens wrote:
> The switch does not support per port MTU setting, but only a MRU
> setting. Implement this by setting the MTU on the CPU port.
> 
> Without this patch the MRU was always set to 1536, not it is set by the

s/not/now/

> DSA subsystem and the user scan change it.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index be64cfdeccc7..f9b690251155 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port,
>  	return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask);
>  }
>  
> +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port,
> +				     int new_mtu)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct realtek_priv *priv = ds->priv;
> +	int length;
> +
> +	/* When a new MTU is set, DSA always set the CPU port's MTU to the

s/set/sets/

> +	 * largest MTU of the slave ports. Because the switch only has a global
> +	 * RX length register, only allowing CPU port here is enough.
> +	 */
> +	if (!dsa_is_cpu_port(ds, port))
> +		return 0;
> +
> +	length = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> +	length += dp->tag_ops->needed_headroom;
> +	length += dp->tag_ops->needed_tailroom;
> +
> +	if (length > RTL8365MB_CFG0_MAX_LEN_MASK)
> +		return -EINVAL;
> +
> +	return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> +				  RTL8365MB_CFG0_MAX_LEN_MASK,
> +				  FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK,
> +					     length));
> +}
> +
> +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> +	return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8;

Is this 8 because of the Realtek CPU tag length? Luiz and Andrew had some good
comments regarding this. Otherwise I think the patch is OK.

> +}
> +
>  static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port,
>  				      u32 offset, u32 length, u64 *mibvalue)
>  {
> @@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  		p->index = i;
>  	}
>  
> -	/* Set maximum packet length to 1536 bytes */
> -	ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> -				 RTL8365MB_CFG0_MAX_LEN_MASK,
> -				 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536));
> -	if (ret)
> -		goto out_teardown_irq;
> -
>  	if (priv->setup_interface) {
>  		ret = priv->setup_interface(ds);
>  		if (ret) {
> @@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>  	.phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
>  	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
>  	.port_stp_state_set = rtl8365mb_port_stp_state_set,
> +	.port_change_mtu = rtl8365mb_port_change_mtu,
> +	.port_max_mtu = rtl8365mb_port_max_mtu,
>  	.get_strings = rtl8365mb_get_strings,
>  	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
>  	.get_sset_count = rtl8365mb_get_sset_count,
> @@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
>  	.phy_read = rtl8365mb_dsa_phy_read,
>  	.phy_write = rtl8365mb_dsa_phy_write,
>  	.port_stp_state_set = rtl8365mb_port_stp_state_set,
> +	.port_change_mtu = rtl8365mb_port_change_mtu,
> +	.port_max_mtu = rtl8365mb_port_max_mtu,
>  	.get_strings = rtl8365mb_get_strings,
>  	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
>  	.get_sset_count = rtl8365mb_get_sset_count,
> -- 
> 2.30.2
>

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Alvin Šipraga]

Hi Hauke,

Thanks for your series.

On Mon, May 09, 2022 at 12:48:44AM +0200, Hauke Mehrtens wrote:
> This was tested on a Buffalo WSR-2533DHP2. This is a board using a 
> Mediatek MT7622 SoC and its 2.5G Ethernet MAC connected over a 2.5G 
> HSGMII link to a RTL8367S switch providing 5 1G ports.
> This is the only board I have using this switch.
> 
> With the DSA_TAG_PROTO_RTL8_4 tag format the TCP checksum for all TCP 
> send packets is wrong. It is set to 0x83c6. The mac driver probably 
> should do some TCP checksum offload, but it does not work. 
> 
> When I used the DSA_TAG_PROTO_RTL8_4T tag format the send packets are 
> ok, but it looks like the system does TCP Large receive offload, but 
> does not update the TCP checksum correctly. I see that multiple received 
> TCP packets are combined into one (using tcpdump on switch port on 
> device). The switch tag is also correctly removed. tcpdump complains
> that the checksum is wrong, it was updated somewhere, but it is wrong.
> 
> Does anyone know what could be wrong here and how to fix this?
> 
> This uses the rtl8367s-sgmii.bin firmware file. I extracted it from a 
> GPL driver source code with a GPL notice on top. I do not have the 
> source code of this firmware. You can download it here:
> https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.bin
> Here are some information about the source:
> https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.txt
> 
> This file does not look like intentional GPL. It would be nice if 
> Realtek could send this file or a similar version to the linux-firmware 
> repository under a license which allows redistribution. I do not have 
> any contact at Realtek, if someone has a contact there it would be nice 
> if we can help me on this topic.

Let me follow up on this. It will take some days so please be patient.

However, in the worst case, I think that the license header in that file is
pretty explicit: You did not enter any particular licensing agreement with
Realtek, and therefore the code (including those bytes of SGMII firmware) are
licensed to you under the GPL. IANAL but it seems quite clear, no?

> 
> Hauke Mehrtens (4):
>   net: dsa: realtek: rtl8365mb: Fix interface type mask
>   net: dsa: realtek: rtl8365mb: Get chip option
>   net: dsa: realtek: rtl8365mb: Add setting MTU
>   net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
> 
>  drivers/net/dsa/realtek/rtl8365mb.c | 444 ++++++++++++++++++++++++++--
>  1 file changed, 413 insertions(+), 31 deletions(-)
> 
> -- 
> 2.30.2
>

Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Vladimir Oltean]

On Mon, May 09, 2022 at 12:48:48AM +0200, Hauke Mehrtens wrote:
> This adds support for SGMII and HSGMII on RTL8367S switches.
> HSGMII is configured using the 2500BASEX mode.
> This is baed on the rtl8367c driver found in OpenWrt.
> 
> For (H)SGMII mode we have to load a firmware into some memory which gets
> executed on the integrated 8051. The firmware binary was added as a
> array into the driver with a GPL license notice on top.
> 
> This was tested on RTL8367S (ver=0x00a0, opt=0x0001).
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 356 +++++++++++++++++++++++++++-
>  1 file changed, 345 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index f9b690251155..5504a34fffeb 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
>   * Copyright (C) 2021 Michael Rasmussen <mir@bang-olufsen.dk>
> + * Copyright (C) 2022 Hauke Mehrtens <hauke@hauke-m.de>
>   *
>   * The RTL8365MB-VC is a 4+1 port 10/100/1000M switch controller. It includes 4
>   * integrated PHYs for the user facing ports, and an extension interface which
> @@ -98,6 +99,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/regmap.h>
>  #include <linux/if_bridge.h>
> +#include <linux/firmware.h>
>  
>  #include "realtek.h"
>  
> @@ -135,6 +137,7 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  
>  /* Chip reset register */
>  #define RTL8365MB_CHIP_RESET_REG	0x1322
> +#define RTL8365MB_CHIP_RESET_DW8051	BIT(4)
>  #define RTL8365MB_CHIP_RESET_SW_MASK	0x0002
>  #define RTL8365MB_CHIP_RESET_HW_MASK	0x0001
>  
> @@ -278,6 +281,29 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK		0x0004
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK		0x0003
>  
> +#define RTL8365MB_SDS_MISC			0x1d11
> +#define  RTL8365MB_CFG_SGMII_RXFC		0x4000
> +#define  RTL8365MB_CFG_SGMII_TXFC		0x2000
> +#define  RTL8365MB_INB_ARB			0x1000
> +#define  RTL8365MB_CFG_MAC8_SEL_HSGMII		0x0800
> +#define  RTL8365MB_CFG_SGMII_FDUP		0x0400
> +#define  RTL8365MB_CFG_SGMII_LINK		0x0200
> +#define  RTL8365MB_CFG_SGMII_SPD		0x0180
> +#define  RTL8365MB_CFG_MAC8_SEL_SGMII		0x0040
> +#define  RTL8365MB_CFG_INB_SEL			0x0038
> +#define  RTL8365MB_CFG_SDS_MODE_18C		0x0007
> +
> +#define RTL8365MB_SDS_INDACS_CMD		0x6600
> +#define RTL8365MB_SDS_INDACS_ADR		0x6601
> +#define RTL8365MB_SDS_INDACS_DATA		0x6602
> +
> +#define RTL8365MB_MISC_CFG0			0x130c
> +#define  RTL8365MB_MISC_CFG0_DW8051_EN		BIT(5)
> +
> +#define RTL8365MB_DW8051_RDY			0x1336
> +#define  RTL8365MB_DW8051_RDY_IROM_MSB		BIT(2)
> +#define  RTL8365MB_DW8051_RDY_ACS_IROM_EN	BIT(1)
> +
>  /* CPU port mask register - controls which ports are treated as CPU ports */
>  #define RTL8365MB_CPU_PORT_MASK_REG	0x1219
>  #define   RTL8365MB_CPU_PORT_MASK_MASK	0x07FF
> @@ -296,6 +322,8 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  #define RTL8365MB_CFG0_MAX_LEN_REG	0x088C
>  #define   RTL8365MB_CFG0_MAX_LEN_MASK	0x3FFF
>  
> +#define RTL8365MB_BYPASS_LINE_RATE		0x03f7
> +
>  /* Port learning limit registers */
>  #define RTL8365MB_LUT_PORT_LEARN_LIMIT_BASE		0x0A20
>  #define RTL8365MB_LUT_PORT_LEARN_LIMIT_REG(_physport) \
> @@ -493,6 +521,39 @@ static const struct rtl8365mb_jam_tbl_entry rtl8365mb_init_jam_common[] = {
>  	{ 0x1D32, 0x0002 },
>  };
>  
> +struct rtl8365mb_sds_init {
> +	unsigned int data;
> +	unsigned int addr;
> +};
> +
> +static const struct rtl8365mb_sds_init redData[] = {
> +	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x21A2, 0x0482}, {0x6960, 0x0483},
> +	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redDataSB[] = {
> +	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x31A2, 0x0482}, {0x6960, 0x0483},
> +	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redData1_5_6[] = {
> +	{0x82F1, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redData8_9[] = {
> +	{0x82F1, 0x0500}, {0xF995, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redDataHB[] = {
> +	{0x82F0, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x7960, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
>  enum rtl8365mb_stp_state {
>  	RTL8365MB_STP_STATE_DISABLED = 0,
>  	RTL8365MB_STP_STATE_BLOCKING = 1,
> @@ -801,6 +862,232 @@ rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
>  	return DSA_TAG_PROTO_RTL8_4;
>  }
>  
> +static int rtl8365mb_sds_indacs_write(struct realtek_priv *priv, unsigned int addr,
> +				      unsigned int data)
> +{
> +	int ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);
> +}
> +
> +static int rtl8365mb_sds_indacs_read(struct realtek_priv *priv, unsigned int addr,
> +				     unsigned int *data)
> +{
> +	int ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, *data);
> +}
> +
> +static int rtl8365mb_ext_init_sgmii_fw(struct realtek_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	const struct firmware *fw;
> +	int ret;
> +	int i;
> +
> +	ret = request_firmware(&fw, "rtl_switch/rtl8367s-sgmii.bin", dev);
> +	if (ret) {
> +		dev_err(dev, "failed to load firmware rtl_switch/rtl8367s-sgmii.bin, error: %i\n",

Can you put the firmware name here and in the MODULE_FIRMWARE definition
behind a common macro?

> +			ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
> +				 RTL8365MB_CHIP_RESET_DW8051,
> +				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
> +				 RTL8365MB_MISC_CFG0_DW8051_EN,
> +				 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_IROM_MSB,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));
> +	if (ret)
> +		goto release_fw;
> +
> +	for (i = 0; i < fw->size; i++) {
> +		ret = regmap_write(priv->map, 0xE000 + i, fw->data[i]);
> +		if (ret)
> +			goto release_fw;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_IROM_MSB,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 0));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
> +				 RTL8365MB_CHIP_RESET_DW8051,
> +				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 0));
> +
> +release_fw:
> +	release_firmware(fw);
> +	return ret;
> +}
> +
> +static int rtl8365mb_ext_init_sgmii(struct realtek_priv *priv, int port, phy_interface_t interface)
> +{
> +	struct rtl8365mb *mb;
> +	int interface_mode;
> +	int sds_mode;
> +	const struct rtl8365mb_sds_init *sds_init;
> +	size_t sds_init_len;
> +	int ext_int;
> +	int ret;
> +	int i;
> +	int val;
> +	int mask;

Please keep variables sorted longest to shortest.

> +
> +	mb = priv->chip_data;
> +
> +	if (mb->chip_id != RTL8365MB_CHIP_ID_8365MB_VC)
> +		return -EINVAL;
> +
> +	ext_int = rtl8365mb_extint_port_map[port];
> +	if (ext_int != 1)
> +		return -EINVAL;
> +
> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
> +		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_SGMII, 1);
> +		interface_mode = RTL8365MB_EXT_PORT_MODE_SGMII;
> +
> +		if (mb->chip_option == 0) {
> +			sds_init = redData;
> +			sds_init_len = ARRAY_SIZE(redData);
> +		} else {
> +			sds_init = redDataSB;
> +			sds_init_len = ARRAY_SIZE(redDataSB);
> +		}
> +	} else if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_HSGMII, 1);
> +		interface_mode = RTL8365MB_EXT_PORT_MODE_HSGMII;
> +
> +		if (mb->chip_option == 0) {
> +			switch (mb->chip_ver & 0x00F0) {
> +			case 0x0010:
> +			case 0x0050:
> +			case 0x0060:
> +				sds_init = redData1_5_6;
> +				sds_init_len = ARRAY_SIZE(redData1_5_6);
> +				break;
> +			case 0x0080:
> +			case 0x0090:
> +				sds_init = redData8_9;
> +				sds_init_len = ARRAY_SIZE(redData8_9);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		} else {
> +			sds_init = redDataHB;
> +			sds_init_len = ARRAY_SIZE(redDataHB);
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < sds_init_len; i++) {
> +		ret = rtl8365mb_sds_indacs_write(priv, sds_init[i].addr, sds_init[i].data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	mask = RTL8365MB_CFG_MAC8_SEL_SGMII | RTL8365MB_CFG_MAC8_SEL_HSGMII;
> +	ret = regmap_update_bits(priv->map,
> +				 RTL8365MB_SDS_MISC,
> +				 mask,
> +				 sds_mode);
> +	if (ret)
> +		return ret;
> +
> +	mask = RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(ext_int);
> +	val = interface_mode << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(ext_int);
> +	ret = regmap_update_bits(priv->map,
> +				 RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(ext_int),
> +				 mask,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_BYPASS_LINE_RATE, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Serdes not reset */
> +	ret = rtl8365mb_sds_indacs_write(priv, 0x0003, 0x7106);
> +	if (ret)
> +		return ret;
> +
> +	return rtl8365mb_ext_init_sgmii_fw(priv);
> +}
> +
> +static int rtl8365mb_ext_sgmii_nway(struct realtek_priv *priv, bool state)

What is "nway"? In-band autoneg by any chance? If so, could you pass
"phylink_autoneg_inband(mode)" rather than "false" as "state"?

> +{
> +	u32 running;
> +	u32 regValue;
> +	int ret;
> +
> +	ret = regmap_read(priv->map, RTL8365MB_MISC_CFG0, &running);
> +	if (running & RTL8365MB_MISC_CFG0_DW8051_EN) {
> +		ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
> +					 RTL8365MB_MISC_CFG0_DW8051_EN,
> +					 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 0));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = rtl8365mb_sds_indacs_read(priv, 0x0002, &regValue);
> +	if (ret)
> +		return ret;
> +
> +	if (state)
> +		regValue |= 0x0200;
> +	else
> +		regValue &= ~0x0200;

No camelCase please.

> +	regValue |= 0x0100;
> +
> +	ret = rtl8365mb_sds_indacs_write(priv, 0x0002, regValue);

Any idea what the magic numbers are? At least 0x0200 looks like BIT(9).

> +	if (ret)
> +		return ret;
> +	return regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
> +				  RTL8365MB_MISC_CFG0_DW8051_EN,
> +				  FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
> +}
> +
>  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  				      phy_interface_t interface)
>  {
> @@ -886,6 +1173,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  }
>  
>  static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
> +					  phy_interface_t interface,
>  					  bool link, int speed, int duplex,
>  					  bool tx_pause, bool rx_pause)
>  {
> @@ -911,7 +1199,7 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  		r_rx_pause = rx_pause ? 1 : 0;
>  		r_tx_pause = tx_pause ? 1 : 0;
>  
> -		if (speed == SPEED_1000) {
> +		if (speed == SPEED_1000 || speed == SPEED_2500) {
>  			r_speed = RTL8365MB_PORT_SPEED_1000M;
>  		} else if (speed == SPEED_100) {
>  			r_speed = RTL8365MB_PORT_SPEED_100M;
> @@ -941,6 +1229,25 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  		r_duplex = 0;
>  	}
>  
> +	if (interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		val = FIELD_PREP(RTL8365MB_CFG_SGMII_FDUP, r_duplex) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_SPD, r_speed) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_LINK, r_link) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_TXFC, r_tx_pause) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_RXFC, r_rx_pause);
> +		ret = regmap_update_bits(priv->map,
> +					 RTL8365MB_SDS_MISC,
> +					 RTL8365MB_CFG_SGMII_FDUP |
> +					 RTL8365MB_CFG_SGMII_SPD |
> +					 RTL8365MB_CFG_SGMII_LINK |
> +					 RTL8365MB_CFG_SGMII_TXFC |
> +					 RTL8365MB_CFG_SGMII_RXFC,
> +					 val);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	val = FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK, 1) |
>  	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK,
>  			 r_tx_pause) |
> @@ -972,10 +1279,15 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
>  	     interface == PHY_INTERFACE_MODE_GMII))
>  		/* Internal PHY */
>  		return true;
> -	else if ((ext_int >= 1) &&
> -		 phy_interface_mode_is_rgmii(interface))
> +	else if ((ext_int == 1) &&
> +		 (phy_interface_mode_is_rgmii(interface) ||
> +		  interface == PHY_INTERFACE_MODE_SGMII ||
> +		  interface == PHY_INTERFACE_MODE_2500BASEX))
>  		/* Extension MAC */
>  		return true;
> +	else if ((ext_int >= 2) &&
> +		 phy_interface_mode_is_rgmii(interface))
> +		return true;
>  
>  	return false;
>  }
> @@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port))
> +	int ext_int = rtl8365mb_extint_port_map[port];
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD;
> +
> +	if (dsa_is_user_port(ds, port)) {
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
> -	else if (dsa_is_cpu_port(ds, port))
> +	} else if (dsa_is_cpu_port(ds, port)) {

What does the quality of being a user port or a CPU port have to do with
which interfaces are supported?

> +		if (ext_int == 1) {
> +			__set_bit(PHY_INTERFACE_MODE_SGMII,
> +				  config->supported_interfaces);
> +			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +				  config->supported_interfaces);
> +			config->mac_capabilities |= MAC_2500FD;
> +		}
>  		phy_interface_set_rgmii(config->supported_interfaces);
> +	}
>  
> -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> -				   MAC_10 | MAC_100 | MAC_1000FD;
>  }
>  
>  static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
> @@ -1020,6 +1343,10 @@ static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
>  				"failed to configure RGMII mode on port %d: %d\n",
>  				port, ret);
>  		return;
> +	} else if (state->interface == PHY_INTERFACE_MODE_SGMII ||
> +		   state->interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		rtl8365mb_ext_init_sgmii(priv, port, state->interface);
> +		rtl8365mb_ext_sgmii_nway(priv, false);
>  	}
>  
>  	/* TODO: Implement MII and RMII modes, which the RTL8365MB-VC also
> @@ -1040,8 +1367,11 @@ static void rtl8365mb_phylink_mac_link_down(struct dsa_switch *ds, int port,
>  	p = &mb->ports[port];
>  	cancel_delayed_work_sync(&p->mib_work);
>  
> -	if (phy_interface_mode_is_rgmii(interface)) {
> -		ret = rtl8365mb_ext_config_forcemode(priv, port, false, 0, 0,
> +	if (phy_interface_mode_is_rgmii(interface) ||
> +	    interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {

If in-band autoneg is possible, there will have to be an additional
condition here, to only force the mode if it is disabled (which it now is)

> +		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
> +						     false, 0, 0,
>  						     false, false);
>  		if (ret)
>  			dev_err(priv->dev,
> @@ -1068,8 +1398,11 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
>  	p = &mb->ports[port];
>  	schedule_delayed_work(&p->mib_work, 0);
>  
> -	if (phy_interface_mode_is_rgmii(interface)) {
> -		ret = rtl8365mb_ext_config_forcemode(priv, port, true, speed,
> +	if (phy_interface_mode_is_rgmii(interface) ||
> +	    interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
> +						     true, speed,
>  						     duplex, tx_pause,
>  						     rx_pause);
>  		if (ret)
> @@ -2156,6 +2489,7 @@ const struct realtek_variant rtl8365mb_variant = {
>  };
>  EXPORT_SYMBOL_GPL(rtl8365mb_variant);
>  
> +MODULE_FIRMWARE("rtl_switch/rtl8367s-sgmii.bin");
>  MODULE_AUTHOR("Alvin Šipraga <alsi@bang-olufsen.dk>");
>  MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch");
>  MODULE_LICENSE("GPL");
> -- 
> 2.30.2
> 

Re: [PATCH 3/4] net: dsa: realtek: rtl8365mb: Add setting MTU

[Vladimir Oltean]

On Mon, May 09, 2022 at 12:48:47AM +0200, Hauke Mehrtens wrote:
> The switch does not support per port MTU setting, but only a MRU
> setting. Implement this by setting the MTU on the CPU port.

Could you also please set ds->mtu_enforcement_ingress, for no other
reason than to have the dev->mtu of all other bridge ports automatically
updated when the user changes one of them?

> 
> Without this patch the MRU was always set to 1536, not it is set by the
> DSA subsystem and the user scan change it.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 43 ++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index be64cfdeccc7..f9b690251155 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -1132,6 +1132,38 @@ static int rtl8365mb_port_set_isolation(struct realtek_priv *priv, int port,
>  	return regmap_write(priv->map, RTL8365MB_PORT_ISOLATION_REG(port), mask);
>  }
>  
> +static int rtl8365mb_port_change_mtu(struct dsa_switch *ds, int port,
> +				     int new_mtu)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct realtek_priv *priv = ds->priv;
> +	int length;
> +
> +	/* When a new MTU is set, DSA always set the CPU port's MTU to the
> +	 * largest MTU of the slave ports. Because the switch only has a global
> +	 * RX length register, only allowing CPU port here is enough.
> +	 */
> +	if (!dsa_is_cpu_port(ds, port))

das_port_is_cpu(dp)

> +		return 0;
> +
> +	length = new_mtu + ETH_HLEN + ETH_FCS_LEN;
> +	length += dp->tag_ops->needed_headroom;
> +	length += dp->tag_ops->needed_tailroom;
> +
> +	if (length > RTL8365MB_CFG0_MAX_LEN_MASK)
> +		return -EINVAL;
> +
> +	return regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> +				  RTL8365MB_CFG0_MAX_LEN_MASK,
> +				  FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK,
> +					     length));
> +}
> +
> +static int rtl8365mb_port_max_mtu(struct dsa_switch *ds, int port)
> +{
> +	return RTL8365MB_CFG0_MAX_LEN_MASK - ETH_HLEN - ETH_FCS_LEN - 8;
> +}
> +
>  static int rtl8365mb_mib_counter_read(struct realtek_priv *priv, int port,
>  				      u32 offset, u32 length, u64 *mibvalue)
>  {
> @@ -1928,13 +1960,6 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  		p->index = i;
>  	}
>  
> -	/* Set maximum packet length to 1536 bytes */
> -	ret = regmap_update_bits(priv->map, RTL8365MB_CFG0_MAX_LEN_REG,
> -				 RTL8365MB_CFG0_MAX_LEN_MASK,
> -				 FIELD_PREP(RTL8365MB_CFG0_MAX_LEN_MASK, 1536));
> -	if (ret)
> -		goto out_teardown_irq;
> -
>  	if (priv->setup_interface) {
>  		ret = priv->setup_interface(ds);
>  		if (ret) {
> @@ -2080,6 +2105,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>  	.phylink_mac_link_down = rtl8365mb_phylink_mac_link_down,
>  	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
>  	.port_stp_state_set = rtl8365mb_port_stp_state_set,
> +	.port_change_mtu = rtl8365mb_port_change_mtu,
> +	.port_max_mtu = rtl8365mb_port_max_mtu,
>  	.get_strings = rtl8365mb_get_strings,
>  	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
>  	.get_sset_count = rtl8365mb_get_sset_count,
> @@ -2101,6 +2128,8 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
>  	.phy_read = rtl8365mb_dsa_phy_read,
>  	.phy_write = rtl8365mb_dsa_phy_write,
>  	.port_stp_state_set = rtl8365mb_port_stp_state_set,
> +	.port_change_mtu = rtl8365mb_port_change_mtu,
> +	.port_max_mtu = rtl8365mb_port_max_mtu,
>  	.get_strings = rtl8365mb_get_strings,
>  	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
>  	.get_sset_count = rtl8365mb_get_sset_count,
> -- 
> 2.30.2
> 

Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Alvin Šipraga]

On Mon, May 09, 2022 at 12:48:48AM +0200, Hauke Mehrtens wrote:
> This adds support for SGMII and HSGMII on RTL8367S switches.
> HSGMII is configured using the 2500BASEX mode.
> This is baed on the rtl8367c driver found in OpenWrt.
> 
> For (H)SGMII mode we have to load a firmware into some memory which gets
> executed on the integrated 8051. The firmware binary was added as a
> array into the driver with a GPL license notice on top.
> 
> This was tested on RTL8367S (ver=0x00a0, opt=0x0001).
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 356 +++++++++++++++++++++++++++-
>  1 file changed, 345 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index f9b690251155..5504a34fffeb 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2021 Alvin Šipraga <alsi@bang-olufsen.dk>
>   * Copyright (C) 2021 Michael Rasmussen <mir@bang-olufsen.dk>
> + * Copyright (C) 2022 Hauke Mehrtens <hauke@hauke-m.de>
>   *
>   * The RTL8365MB-VC is a 4+1 port 10/100/1000M switch controller. It includes 4
>   * integrated PHYs for the user facing ports, and an extension interface which
> @@ -98,6 +99,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/regmap.h>
>  #include <linux/if_bridge.h>
> +#include <linux/firmware.h>
>  
>  #include "realtek.h"
>  
> @@ -135,6 +137,7 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  
>  /* Chip reset register */
>  #define RTL8365MB_CHIP_RESET_REG	0x1322
> +#define RTL8365MB_CHIP_RESET_DW8051	BIT(4)

Please stick with the convention and use 0x0010.

>  #define RTL8365MB_CHIP_RESET_SW_MASK	0x0002
>  #define RTL8365MB_CHIP_RESET_HW_MASK	0x0001
>  
> @@ -278,6 +281,29 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK		0x0004
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK		0x0003
>  
> +#define RTL8365MB_SDS_MISC			0x1d11
> +#define  RTL8365MB_CFG_SGMII_RXFC		0x4000

Please stick to the existing naming conventions of the Linux driver rather than
the Realtek (OpenWrt) one. The convention is roughly something like this:

                  reg name   _REG because it's a register 
                  vvvvvvvvvv vvv
#define RTL8365MB_COOL_STUFF_REG			0xC001 < all-caps hex
#define   RTL8365MB_COOL_STUFF_RINGTONE_SELECT_MASK	0x8000
        ^^          ^^^^^^^^^^ ^^^^^^^^^^^^^^^ ^^^^
  2 space indent    reg name   field name      _MASK because it's a mask

You are of course at liberty to use a different register name to the Realtek
driver if you think it fits better.

> +#define  RTL8365MB_CFG_SGMII_TXFC		0x2000
> +#define  RTL8365MB_INB_ARB			0x1000
> +#define  RTL8365MB_CFG_MAC8_SEL_HSGMII		0x0800
> +#define  RTL8365MB_CFG_SGMII_FDUP		0x0400
> +#define  RTL8365MB_CFG_SGMII_LINK		0x0200
> +#define  RTL8365MB_CFG_SGMII_SPD		0x0180
> +#define  RTL8365MB_CFG_MAC8_SEL_SGMII		0x0040
> +#define  RTL8365MB_CFG_INB_SEL			0x0038
> +#define  RTL8365MB_CFG_SDS_MODE_18C		0x0007
> +
> +#define RTL8365MB_SDS_INDACS_CMD		0x6600
> +#define RTL8365MB_SDS_INDACS_ADR		0x6601
> +#define RTL8365MB_SDS_INDACS_DATA		0x6602
> +
> +#define RTL8365MB_MISC_CFG0			0x130c
> +#define  RTL8365MB_MISC_CFG0_DW8051_EN		BIT(5)
> +
> +#define RTL8365MB_DW8051_RDY			0x1336
> +#define  RTL8365MB_DW8051_RDY_IROM_MSB		BIT(2)
> +#define  RTL8365MB_DW8051_RDY_ACS_IROM_EN	BIT(1)

Similar comments apply here, registers with _REG, masks with _MASK, no BIT(),
more indentation, etc.

Also, if you know a little bit about what the register(s) do, a comment is
always appreciated. For example I had a look at this register map from Realtek:

#define    RTL8367C_REG_DW8051_RDY    0x1336
#define    RTL8367C_VIAROM_WRITE_EN_OFFSET    9
#define    RTL8367C_VIAROM_WRITE_EN_MASK    0x200
#define    RTL8367C_SPIF_CK2_OFFSET    8
#define    RTL8367C_SPIF_CK2_MASK    0x100
#define    RTL8367C_RRCP_MDOE_OFFSET    7
#define    RTL8367C_RRCP_MDOE_MASK    0x80
#define    RTL8367C_DW8051_RATE_OFFSET    4
#define    RTL8367C_DW8051_RATE_MASK    0x70
#define    RTL8367C_IROM_MSB_OFFSET    2
#define    RTL8367C_IROM_MSB_MASK    0xC
#define    RTL8367C_ACS_IROM_ENABLE_OFFSET    1
#define    RTL8367C_ACS_IROM_ENABLE_MASK    0x2
#define    RTL8367C_DW8051_READY_OFFSET    0
#define    RTL8367C_DW8051_READY_MASK    0x1

Here it would appear that the register was originally called DW8051_READY
because it contained a single READY bit at offset 0. Then Realtek added some
other stuff completely unrelated to readiness. So you might want to call it
something more generic like RTL8365MB_DW8051_REG. And a comment explaining what
an 8051 is doing here, etc.

> +
>  /* CPU port mask register - controls which ports are treated as CPU ports */
>  #define RTL8365MB_CPU_PORT_MASK_REG	0x1219
>  #define   RTL8365MB_CPU_PORT_MASK_MASK	0x07FF
> @@ -296,6 +322,8 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  #define RTL8365MB_CFG0_MAX_LEN_REG	0x088C
>  #define   RTL8365MB_CFG0_MAX_LEN_MASK	0x3FFF
>  
> +#define RTL8365MB_BYPASS_LINE_RATE 0x03f7

Please document the type of value this can hold by specifying the relevant
masks. Also you could point out that it is only used for TMII mode (Turbo MII?).

> +
>  /* Port learning limit registers */
>  #define RTL8365MB_LUT_PORT_LEARN_LIMIT_BASE		0x0A20
>  #define RTL8365MB_LUT_PORT_LEARN_LIMIT_REG(_physport) \
> @@ -493,6 +521,39 @@ static const struct rtl8365mb_jam_tbl_entry rtl8365mb_init_jam_common[] = {
>  	{ 0x1D32, 0x0002 },
>  };
>  
> +struct rtl8365mb_sds_init {
> +	unsigned int data;
> +	unsigned int addr;
> +};

I would swap the order here to be consistent with struct
rtl8365mb_jam_tbl_entry. Also make it u16 since the values are truly 16 bit.

Actually, this is just a jam table entry, just for a different register map (via
serdes indirect access). Maybe you can just follow the same model as the regular
jam tables and add a const pointer to one of your arrays below based on the
option in the rtl8365mb_detect function?

> +
> +static const struct rtl8365mb_sds_init redData[] = {

What does redData stand for? Also we don't use camelCase in the driver.

> +	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x21A2, 0x0482}, {0x6960, 0x0483},
> +	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redDataSB[] = {
> +	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x31A2, 0x0482}, {0x6960, 0x0483},
> +	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redData1_5_6[] = {
> +	{0x82F1, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redData8_9[] = {
> +	{0x82F1, 0x0500}, {0xF995, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redDataHB[] = {

How about SB/HB?

> +	{0x82F0, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x7960, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
>  enum rtl8365mb_stp_state {
>  	RTL8365MB_STP_STATE_DISABLED = 0,
>  	RTL8365MB_STP_STATE_BLOCKING = 1,
> @@ -801,6 +862,232 @@ rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
>  	return DSA_TAG_PROTO_RTL8_4;
>  }
>  
> +static int rtl8365mb_sds_indacs_writee(struct realtek_priv *priv, unsigned int addr,
> +				      unsigned int data)

Likewise I would make these unsigned ints into u16's, and why not just
serdes_read/write? That it is indirect access (indacs) is not really relevant.

> +{
> +	int ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);

Sorry to drone on about naming but since we use the abbreviation addr here, the
register name ought to also be ADDR and not ADR.

> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);

Avoid magic values by populating the relevant macro fields:

#define    RTL8367C_REG_SDS_INDACS_CMD    0x6600
#define    RTL8367C_SDS_CMD_BUSY_OFFSET    8
#define    RTL8367C_SDS_CMD_BUSY_MASK    0x100
#define    RTL8367C_SDS_CMD_OFFSET    7
#define    RTL8367C_SDS_CMD_MASK    0x80
#define    RTL8367C_SDS_RWOP_OFFSET    6
#define    RTL8367C_SDS_RWOP_MASK    0x40
#define    RTL8367C_SDS_INDEX_OFFSET    0
#define    RTL8367C_SDS_INDEX_MASK    0x3F

So 0x00C0 would correspond to RWOP=1 and CMD=1 I think. This is similar to the
indirect access for the PHY registers. From rtl8365mb_phy_ocp_write():

	/* Execute write operation */
	val = FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_MASK,
			 RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_VALUE) |
	      FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_RW_MASK,
			 RTL8365MB_INDIRECT_ACCESS_CTRL_RW_WRITE);
	ret = regmap_write(priv->map_nolock, RTL8365MB_INDIRECT_ACCESS_CTRL_REG,
			   val);

So maybe you can take inspiration from there.


> +}
> +
> +static int rtl8365mb_sds_indacs_read(struct realtek_priv *priv, unsigned int addr,
> +				     unsigned int *data)
> +{

By the way, we had issues with indirect access of the PHY registers colliding
with ordinary register access on SMP systems and returning nonsense data. Could
you please acquire the priv->map_lock directly in these serdes read/write
functions and use the priv->map_nolock regmap instead? See the phy access
functions for how it works or check the recent git history.

> +	int ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, *data);

Shouldn't this be a read? Did you test the relevant code that relies on serdes
read?

> +}
> +
> +static int rtl8365mb_ext_init_sgmii_fw(struct realtek_priv *priv)
> +{
> +	struct device *dev = priv->dev;

This variable is not really needed

> +	const struct firmware *fw;
> +	int ret;
> +	int i;
> +
> +	ret = request_firmware(&fw, "rtl_switch/rtl8367s-sgmii.bin", dev);

Since the firmware is generic to the whole family, I would name this
rtl8365mb-sgmii.bin, since we have tacitly agreed to refer to the family by
rtl8365mb. Other ideas welcome but this looks a bit odd.

Also, how about not hardcoding the FW name here but putting it in a macro?

> +	if (ret) {
> +		dev_err(dev, "failed to load firmware rtl_switch/rtl8367s-sgmii.bin, error: %i\n",
> +			ret);

AFAIK request_firmware will print errors for you, so you can just return ret; here.

> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
> +				 RTL8365MB_CHIP_RESET_DW8051,
> +				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
> +				 RTL8365MB_MISC_CFG0_DW8051_EN,
> +				 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_IROM_MSB,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));

Can you do this and the last write in one go? Is this timing sensitive somehow?

> +	if (ret)
> +		goto release_fw;
> +
> +	for (i = 0; i < fw->size; i++) {

Perhaps you ought to do a size check on the firmware file?

> +		ret = regmap_write(priv->map, 0xE000 + i, fw->data[i]);

Avoid magic values please :)

> +		if (ret)
> +			goto release_fw;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_IROM_MSB,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 0));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
> +				 RTL8365MB_CHIP_RESET_DW8051,
> +				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 0));

It would be cool to document here in this function that you are actually loading
firmware into the 8051.

> +
> +release_fw:
> +	release_firmware(fw);
> +	return ret;

Newline before the return.

> +}
> +
> +static int rtl8365mb_ext_init_sgmii(struct realtek_priv *priv, int port, phy_interface_t interface)
> +{
> +	struct rtl8365mb *mb;
> +	int interface_mode;
> +	int sds_mode;
> +	const struct rtl8365mb_sds_init *sds_init;
> +	size_t sds_init_len;

Consider putting this in the mb struct like the jam table.

> +	int ext_int;
> +	int ret;
> +	int i;
> +	int val;
> +	int mask;

Also remember to use reverse christmas tree order.

> +
> +	mb = priv->chip_data;
> +
> +	if (mb->chip_id != RTL8365MB_CHIP_ID_8365MB_VC)
> +		return -EINVAL;

Why this check?

> +
> +	ext_int = rtl8365mb_extint_port_map[port];
> +	if (ext_int != 1)
> +		return -EINVAL;

Also this needs some explanation

> +
> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
> +		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_SGMII, 1);
> +		interface_mode = RTL8365MB_EXT_PORT_MODE_SGMII;
> +
> +		if (mb->chip_option == 0) {
> +			sds_init = redData;
> +			sds_init_len = ARRAY_SIZE(redData);
> +		} else {
> +			sds_init = redDataSB;
> +			sds_init_len = ARRAY_SIZE(redDataSB);
> +		}
> +	} else if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_HSGMII, 1);
> +		interface_mode = RTL8365MB_EXT_PORT_MODE_HSGMII;
> +
> +		if (mb->chip_option == 0) {
> +			switch (mb->chip_ver & 0x00F0) {

Rather than magic masks like 0x00F0, why not just extend the _MASKs under the
CHIP_VER_REG macro? Here is what the vendor driver says the subfields are:

#define    RTL8367C_REG_CHIP_VER    0x1301
#define    RTL8367C_VERID_OFFSET    12
#define    RTL8367C_VERID_MASK    0xF000
#define    RTL8367C_MCID_OFFSET    8
#define    RTL8367C_MCID_MASK    0xF00
#define    RTL8367C_MODEL_ID_OFFSET    4
#define    RTL8367C_MODEL_ID_MASK    0xF0
#define    RTL8367C_AFE_VERSION_OFFSET    0
#define    RTL8367C_AFE_VERSION_MASK    0x1

So here you are testing the model and picking a specific serdes init.

(Sadly Realtek said they can't provide me with a mapping of chip ID/ver value
<-> chip name, but perhaps one day we can make better sense of this...)

> +			case 0x0010:
> +			case 0x0050:
> +			case 0x0060:
> +				sds_init = redData1_5_6;
> +				sds_init_len = ARRAY_SIZE(redData1_5_6);
> +				break;
> +			case 0x0080:
> +			case 0x0090:
> +				sds_init = redData8_9;
> +				sds_init_len = ARRAY_SIZE(redData8_9);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		} else {
> +			sds_init = redDataHB;
> +			sds_init_len = ARRAY_SIZE(redDataHB);
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < sds_init_len; i++) {
> +		ret = rtl8365mb_sds_indacs_write(priv, sds_init[i].addr, sds_init[i].data);

Try to stick to 80 columns unless it looks really ugly

> +		if (ret)
> +			return ret;
> +	}
> +
> +	mask = RTL8365MB_CFG_MAC8_SEL_SGMII | RTL8365MB_CFG_MAC8_SEL_HSGMII;
> +	ret = regmap_update_bits(priv->map,
> +				 RTL8365MB_SDS_MISC,
> +				 mask,
> +				 sds_mode);
> +	if (ret)
> +		return ret;

Can the interface be changed at runtime with DSA? (I don't know.) If so then I
think you need to do some cleaning up in the case where (H)SGMII was configured,
and then e.g. RGMII is later configured. e.g. then this SDS_MISC bits SEL_SGMII
or SEL_HSGMII should be zeroed out.

> +
> +	mask = RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(ext_int);
> +	val = interface_mode << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(ext_int);
> +	ret = regmap_update_bits(priv->map,
> +				 RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(ext_int),
> +				 mask,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_BYPASS_LINE_RATE, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Serdes not reset */
> +	ret = rtl8365mb_sds_indacs_write(priv, 0x0003, 0x7106);

If this is random Realtek voodoo you do not understand, please say so rather
than copying the cryptic comment from the driver. That way we know what we don't
know.

> +	if (ret)
> +		return ret;
> +
> +	return rtl8365mb_ext_init_sgmii_fw(priv);

We don't really end functions like this in the driver unless they are (nearly)
one-liners, but rather:

	ret = rtl8365mb_ext_init_sgmii_fw(priv);
	if(ret)
		return ret;

	return 0;

Not a big deal though.

> +}
> +
> +static int rtl8365mb_ext_sgmii_nway(struct realtek_priv *priv, bool state)
> +{
> +	u32 running;
> +	u32 regValue;

Reverse christmas tree & camelCase again. s/regValue/val/ per convention.

> +	int ret;
> +
> +	ret = regmap_read(priv->map, RTL8365MB_MISC_CFG0, &running);

I think this line is a good indication that the register macro is poorly named.

> +	if (running & RTL8365MB_MISC_CFG0_DW8051_EN) {
> +		ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
> +					 RTL8365MB_MISC_CFG0_DW8051_EN,
> +					 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 0));
> +		if (ret)
> +			return ret;
> +	}

Is there any harm in just setting the bit to 0 unconditionally? And can we even
enter this function with the 8051 enabled?

> +
> +	ret = rtl8365mb_sds_indacs_read(priv, 0x0002, &regValue);
> +	if (ret)
> +		return ret;
> +
> +	if (state)
> +		regValue |= 0x0200;
> +	else
> +		regValue &= ~0x0200;
> +	regValue |= 0x0100;

Any idea what serdes register 0x0002 and its value means?

> +
> +	ret = rtl8365mb_sds_indacs_write(priv, 0x0002, regValue);
> +	if (ret)
> +		return ret;
> +	return regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,

Again please don't end functions like this but rather with a return 0;. Also you
are missing a newline.

> +				  RTL8365MB_MISC_CFG0_DW8051_EN,
> +				  FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
> +}
> +
>  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  				      phy_interface_t interface)
>  {
> @@ -886,6 +1173,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  }
>  
>  static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
> +					  phy_interface_t interface,
>  					  bool link, int speed, int duplex,
>  					  bool tx_pause, bool rx_pause)
>  {
> @@ -911,7 +1199,7 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  		r_rx_pause = rx_pause ? 1 : 0;
>  		r_tx_pause = tx_pause ? 1 : 0;
>  
> -		if (speed == SPEED_1000) {
> +		if (speed == SPEED_1000 || speed == SPEED_2500) {
>  			r_speed = RTL8365MB_PORT_SPEED_1000M;

This looks odd, maybe a comment is in order?

>  		} else if (speed == SPEED_100) {
>  			r_speed = RTL8365MB_PORT_SPEED_100M;
> @@ -941,6 +1229,25 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  		r_duplex = 0;
>  	}
>  
> +	if (interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		val = FIELD_PREP(RTL8365MB_CFG_SGMII_FDUP, r_duplex) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_SPD, r_speed) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_LINK, r_link) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_TXFC, r_tx_pause) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_RXFC, r_rx_pause);
> +		ret = regmap_update_bits(priv->map,
> +					 RTL8365MB_SDS_MISC,
> +					 RTL8365MB_CFG_SGMII_FDUP |
> +					 RTL8365MB_CFG_SGMII_SPD |
> +					 RTL8365MB_CFG_SGMII_LINK |
> +					 RTL8365MB_CFG_SGMII_TXFC |
> +					 RTL8365MB_CFG_SGMII_RXFC,
> +					 val);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	val = FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK, 1) |
>  	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK,
>  			 r_tx_pause) |
> @@ -972,10 +1279,15 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
>  	     interface == PHY_INTERFACE_MODE_GMII))
>  		/* Internal PHY */
>  		return true;
> -	else if ((ext_int >= 1) &&
> -		 phy_interface_mode_is_rgmii(interface))
> +	else if ((ext_int == 1) &&

I'm again curious why you limit these changes to EXT port 1.

> +		 (phy_interface_mode_is_rgmii(interface) ||
> +		  interface == PHY_INTERFACE_MODE_SGMII ||
> +		  interface == PHY_INTERFACE_MODE_2500BASEX))
>  		/* Extension MAC */
>  		return true;
> +	else if ((ext_int >= 2) &&
> +		 phy_interface_mode_is_rgmii(interface))
> +		return true;
>  
>  	return false;
>  }
> @@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port))
> +	int ext_int = rtl8365mb_extint_port_map[port];
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD;
> +
> +	if (dsa_is_user_port(ds, port)) {
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
> -	else if (dsa_is_cpu_port(ds, port))
> +	} else if (dsa_is_cpu_port(ds, port)) {
> +		if (ext_int == 1) {
> +			__set_bit(PHY_INTERFACE_MODE_SGMII,
> +				  config->supported_interfaces);
> +			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +				  config->supported_interfaces);
> +			config->mac_capabilities |= MAC_2500FD;
> +		}

I am not satisfied with this check because not all switches in this family
support (H)SGMII. Also testing the ext_int ID is not particularly elucidating to
the reader.

>  		phy_interface_set_rgmii(config->supported_interfaces);

Hmm, I guess this is actually not true for your RTL8367S? Isn't that
(H)SGMII-only on its first extension port? I guess this was a bug already,
though.

> +	}
>  
> -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> -				   MAC_10 | MAC_100 | MAC_1000FD;
>  }
>  
>  static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
> @@ -1020,6 +1343,10 @@ static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
>  				"failed to configure RGMII mode on port %d: %d\n",
>  				port, ret);
>  		return;
> +	} else if (state->interface == PHY_INTERFACE_MODE_SGMII ||
> +		   state->interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		rtl8365mb_ext_init_sgmii(priv, port, state->interface);
> +		rtl8365mb_ext_sgmii_nway(priv, false);
>  	}
>  
>  	/* TODO: Implement MII and RMII modes, which the RTL8365MB-VC also
> @@ -1040,8 +1367,11 @@ static void rtl8365mb_phylink_mac_link_down(struct dsa_switch *ds, int port,
>  	p = &mb->ports[port];
>  	cancel_delayed_work_sync(&p->mib_work);
>  
> -	if (phy_interface_mode_is_rgmii(interface)) {
> -		ret = rtl8365mb_ext_config_forcemode(priv, port, false, 0, 0,
> +	if (phy_interface_mode_is_rgmii(interface) ||
> +	    interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
> +						     false, 0, 0,
>  						     false, false);
>  		if (ret)
>  			dev_err(priv->dev,
> @@ -1068,8 +1398,11 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
>  	p = &mb->ports[port];
>  	schedule_delayed_work(&p->mib_work, 0);
>  
> -	if (phy_interface_mode_is_rgmii(interface)) {
> -		ret = rtl8365mb_ext_config_forcemode(priv, port, true, speed,
> +	if (phy_interface_mode_is_rgmii(interface) ||
> +	    interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
> +						     true, speed,
>  						     duplex, tx_pause,
>  						     rx_pause);
>  		if (ret)
> @@ -2156,6 +2489,7 @@ const struct realtek_variant rtl8365mb_variant = {
>  };
>  EXPORT_SYMBOL_GPL(rtl8365mb_variant);
>  
> +MODULE_FIRMWARE("rtl_switch/rtl8367s-sgmii.bin");

Do you happen to know the kernel workflow for merging patches which depend on
firmware? Should the firmware land before or after this lands in net-next and/or
mainline?

Kind regards,
Alvin

>  MODULE_AUTHOR("Alvin Šipraga <alsi@bang-olufsen.dk>");
>  MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch");
>  MODULE_LICENSE("GPL");
> -- 
> 2.30.2
>

Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Alvin Šipraga]

On Tue, May 10, 2022 at 08:29:10PM +0300, Vladimir Oltean wrote:
> On Mon, May 09, 2022 at 12:48:48AM +0200, Hauke Mehrtens wrote:
> > @@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
> >  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
> >  				       struct phylink_config *config)
> >  {
> > -	if (dsa_is_user_port(ds, port))
> > +	int ext_int = rtl8365mb_extint_port_map[port];
> > +
> > +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> > +				   MAC_10 | MAC_100 | MAC_1000FD;
> > +
> > +	if (dsa_is_user_port(ds, port)) {
> >  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> >  			  config->supported_interfaces);
> > -	else if (dsa_is_cpu_port(ds, port))
> > +	} else if (dsa_is_cpu_port(ds, port)) {
> 
> What does the quality of being a user port or a CPU port have to do with
> which interfaces are supported?

Right, I think this function was actually broken already in a few ways. The
switch will have ports with integrated PHYs, and ports with extension interfaces
like RGMII or SGMII etc. But which of those ports one uses as a CPU port, user
port, or (one day) DSA port, is of no concern to the switch. The supported
interface of a given port is a static property and simply a function of the port
number and switch model. All switch models in the family have between 1 and 2
ports with an extension interface.

Luiz introduced this map:

/* valid for all 6-port or less variants */
static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2, -1, -1};

... which I think is actually what we ought to test on. It can be improved, but
currently it is correct for all supported models.

So something like this would be correct:

static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
				       struct phylink_config *config)
{
	int ext_int = rtl8365mb_extint_port_map[port];
	if (ext_int == -1) {
		/* integrated PHY, set PHY_INTERFACE_MODE_INTERNAL etc. */
	} else {
		/* extension interface available, but here one should really
		 * check the model based on the chip ID/version, because it
		 * varies a lot
		 */
		if (model == RTL8365MB && ext_int == 1)
			/* RGMII */
		else if (model == RTL8367S && ext_int == 1)
			/* SGMII / HSGMII */
		else if (model == RTL8367S && ext_int == 2)
			/* RGMII */
		/* etc */
	}

	/* ... */
}

There are of course various ways to do this.

Hauke, do you follow what I mean?  Would you like me to prepare a patch for the
current supported models/interfaces and then you can rebase your changes on top
of that to indicate support for (H)SGMII? Or do you want to give it a try
yourself?

Kind regards,
Alvin
    

> 
> > +		if (ext_int == 1) {
> > +			__set_bit(PHY_INTERFACE_MODE_SGMII,
> > +				  config->supported_interfaces);
> > +			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +				  config->supported_interfaces);
> > +			config->mac_capabilities |= MAC_2500FD;
> > +		}
> >  		phy_interface_set_rgmii(config->supported_interfaces);
> > +	}
> >  
> > -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> > -				   MAC_10 | MAC_100 | MAC_1000FD;
> >  }
> >  
> >  static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,

Re: [PATCH 0/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Hauke Mehrtens]

On 5/9/22 08:28, Luiz Angelo Daros de Luca wrote:
>> This was tested on a Buffalo WSR-2533DHP2. This is a board using a
>> Mediatek MT7622 SoC and its 2.5G Ethernet MAC connected over a 2.5G
>> HSGMII link to a RTL8367S switch providing 5 1G ports.
>> This is the only board I have using this switch.
>>
>> With the DSA_TAG_PROTO_RTL8_4 tag format the TCP checksum for all TCP
>> send packets is wrong. It is set to 0x83c6. The mac driver probably
>> should do some TCP checksum offload, but it does not work.
> 
> 0x83c6 might be yout tcp pseudo ip header sum.

Yes this is the default checksum. I see the same checksum when listening 
on my laptop for packets it sends out and where the NIC does checksum 
offloading.

>> When I used the DSA_TAG_PROTO_RTL8_4T tag format the send packets are
>> ok, but it looks like the system does TCP Large receive offload, but
>> does not update the TCP checksum correctly. I see that multiple received
>> TCP packets are combined into one (using tcpdump on switch port on
>> device). The switch tag is also correctly removed. tcpdump complains
>> that the checksum is wrong, it was updated somewhere, but it is wrong.
>>
>> Does anyone know what could be wrong here and how to fix this?
> 
> The good news, it is a known problem:
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20220411230305.28951-1-luizluca@gmail.com/
> (there are some interesting discussions)
> https://patchwork.kernel.org/project/netdevbpf/patch/20220416052737.25509-1-luizluca@gmail.com/
> (merged)

Thanks for the links.

> The bad news, there is no way to enable checksum offload with
> mediatek+realtek. You'll need to disable almost any type of offload.
> For any tag before the IP header, if your driver/HW does not support a
> way to set where the IP header starts and the offload HW does not
> understand the tag protocol, the offload HW will keep the pseudo ip
> header sum. And for tags after the payload, the offload HW will blend
> the tag with the payload, generating bad checksums when the switch
> removes the tag.
> 
> You can do that from userland, using ethtool on the master port before
> the bridge is up, or patching the driver. You can try this patch
> (written for MT7620 but it is easy to adapt it to upstream
> mtk_eth_soc.c).
> https://github.com/luizluca/openwrt/commit/d799bd363f902bf3b9c595972a1b9280a0b61dca
> . I never submitted that upstream because I don't have the HW to test
> it (Arinç tested a modified version in an MT7621) and I didn't
> investigate how much those extra ifs in ndo_features_check will cost
> in performance when the driver does support the tag (using a mediatek
> switch).

Thanks for the tip to remove the checksum offloading before adding the 
device to the bridge, I was already wondering why it did not work when I 
deactivated the checksum offloading later.

Thanks for the link. I also have one device with a MT7531 switch, but it 
is sued in production. I will check this in the next days.


> And the DSA_TAG_PROTO_RTL8_4T already paid off. It was added exactly
> as a way to test checksum errors. Probably no offload will work for
> tags that are after the payload if the offload HW does not already
> know that tag (e.g. same vendors). DSA_TAG_PROTO_RTL8_4T works because
> it calculates the checksum in software before the tag is added.
> However, during my tests, I never tested TCP Large receive offload.

I accidentally tested TCP Large receive offload. ;-) The driver has a 
special DMA flag for to tell the MAC that there is a Mediatek tag in the 
packet.

>> This uses the rtl8367s-sgmii.bin firmware file. I extracted it from a
>> GPL driver source code with a GPL notice on top. I do not have the
>> source code of this firmware. You can download it here:
>> https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.bin
>> Here are some information about the source:
>> https://hauke-m.de/files/rtl8367/rtl8367s-sgmii.txt
>>
>> This file does not look like intentional GPL. It would be nice if
>> Realtek could send this file or a similar version to the linux-firmware
>> repository under a license which allows redistribution. I do not have
>> any contact at Realtek, if someone has a contact there it would be nice
>> if we can help me on this topic.
>>
>> Hauke Mehrtens (4):
>>    net: dsa: realtek: rtl8365mb: Fix interface type mask
>>    net: dsa: realtek: rtl8365mb: Get chip option
>>    net: dsa: realtek: rtl8365mb: Add setting MTU
>>    net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support
>>
>>   drivers/net/dsa/realtek/rtl8365mb.c | 444 ++++++++++++++++++++++++++--
>>   1 file changed, 413 insertions(+), 31 deletions(-)

Hauke

Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII support

[Luiz Angelo Daros de Luca]

> On Tue, May 10, 2022 at 08:29:10PM +0300, Vladimir Oltean wrote:
> > On Mon, May 09, 2022 at 12:48:48AM +0200, Hauke Mehrtens wrote:
> > > @@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
> > >  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
> > >                                    struct phylink_config *config)
> > >  {
> > > -   if (dsa_is_user_port(ds, port))
> > > +   int ext_int = rtl8365mb_extint_port_map[port];
> > > +
> > > +   config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> > > +                              MAC_10 | MAC_100 | MAC_1000FD;
> > > +
> > > +   if (dsa_is_user_port(ds, port)) {
> > >             __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > >                       config->supported_interfaces);
> > > -   else if (dsa_is_cpu_port(ds, port))
> > > +   } else if (dsa_is_cpu_port(ds, port)) {
> >
> > What does the quality of being a user port or a CPU port have to do with
> > which interfaces are supported?
>
> Right, I think this function was actually broken already in a few ways. The
> switch will have ports with integrated PHYs, and ports with extension interfaces
> like RGMII or SGMII etc. But which of those ports one uses as a CPU port, user
> port, or (one day) DSA port, is of no concern to the switch. The supported
> interface of a given port is a static property and simply a function of the port
> number and switch model. All switch models in the family have between 1 and 2
> ports with an extension interface.
>
> Luiz introduced this map:
>
> /* valid for all 6-port or less variants */
> static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2, -1, -1};
>
> ... which I think is actually what we ought to test on. It can be improved, but
> currently it is correct for all supported models.

I was playing some time ago with a more detailed description of
ports/supported modes I manually extracted from Realtek docs:

https://github.com/luizluca/linux/commit/d64201989803274bf6ba8bb784e2bf500114cff5

It might have more details than the driver really needs. Although
there are a lot of new lines with duplicated parts, I don't believe
that this family will grow too much.
I can get this upstream-ready if it looks the way to go.

Another approach would be to check if the switch can describe its
capabilities programmatically. The Realtek rtl8367c code has lots and
comes and goes, reads mysterious registers, but maybe deep down there
might be a way the switch can tell how many ports it has, which one
are really in use and what modes each port does support.

Regards,

Luiz