[PATCH net-next 0/5] TXGBE support more modules

[PATCH net-next 0/5] TXGBE support more modules

[Jiawen Wu]

Support CR modules for 25G devices and QSFP modules for 40G devices. And
implement .get_module_eeprom_by_page() to get module info.

Jiawen Wu (5):
  net: txgbe: support CR modules for AML devices
  net: txgbe: rename the SFP related
  net: txgbe: improve functions of AML 40G devices
  net: txgbe: delay to identify modules in .ndo_open
  net: txgbe: support getting module EEPROM by page

 .../net/ethernet/wangxun/libwx/wx_ethtool.c   |  12 -
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |   2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 264 ++++++++++++++----
 .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |   5 +-
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    |  38 ++-
 .../net/ethernet/wangxun/txgbe/txgbe_irq.c    |  10 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  23 +-
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    |   2 -
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  34 ++-
 9 files changed, 294 insertions(+), 96 deletions(-)

-- 
2.48.1

[PATCH net-next 1/5] net: txgbe: support CR modules for AML devices

[Jiawen Wu]

Support to identify 25G/10G CR modules for AML devices. Autoneg is
enbaled by default in CR mode.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 59 +++++++++++++------
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    |  3 +-
 2 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
index 35eebdb07761..a7650f548fa4 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
@@ -104,7 +104,8 @@ static int txgbe_set_phy_link_hostif(struct wx *wx, int speed, int autoneg, int
 					 WX_HI_COMMAND_TIMEOUT, true);
 }
 
-static void txgbe_get_link_capabilities(struct wx *wx, int *speed, int *duplex)
+static void txgbe_get_link_capabilities(struct wx *wx, int *speed,
+					int *autoneg, int *duplex)
 {
 	struct txgbe *txgbe = wx->priv;
 
@@ -115,6 +116,7 @@ static void txgbe_get_link_capabilities(struct wx *wx, int *speed, int *duplex)
 	else
 		*speed = SPEED_UNKNOWN;
 
+	*autoneg = phylink_test(txgbe->advertising, Autoneg);
 	*duplex = *speed == SPEED_UNKNOWN ? DUPLEX_HALF : DUPLEX_FULL;
 }
 
@@ -135,11 +137,11 @@ static void txgbe_get_mac_link(struct wx *wx, int *speed)
 
 int txgbe_set_phy_link(struct wx *wx)
 {
-	int speed, duplex, err;
+	int speed, autoneg, duplex, err;
 
-	txgbe_get_link_capabilities(wx, &speed, &duplex);
+	txgbe_get_link_capabilities(wx, &speed, &autoneg, &duplex);
 
-	err = txgbe_set_phy_link_hostif(wx, speed, 0, duplex);
+	err = txgbe_set_phy_link_hostif(wx, speed, autoneg, duplex);
 	if (err) {
 		wx_err(wx, "Failed to setup link\n");
 		return err;
@@ -154,19 +156,43 @@ static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sfp_id *id)
 	DECLARE_PHY_INTERFACE_MASK(interfaces);
 	struct txgbe *txgbe = wx->priv;
 
-	if (id->com_25g_code & (TXGBE_SFF_25GBASESR_CAPABLE |
-				TXGBE_SFF_25GBASEER_CAPABLE |
-				TXGBE_SFF_25GBASELR_CAPABLE)) {
-		phylink_set(modes, 25000baseSR_Full);
+	if (id->cable_tech & TXGBE_SFF_DA_PASSIVE_CABLE) {
+		txgbe->link_port = PORT_DA;
+		phylink_set(modes, Autoneg);
+		if (id->com_25g_code == TXGBE_SFF_25GBASECR_91FEC ||
+		    id->com_25g_code == TXGBE_SFF_25GBASECR_74FEC ||
+		    id->com_25g_code == TXGBE_SFF_25GBASECR_NOFEC) {
+			phylink_set(modes, 25000baseCR_Full);
+			phylink_set(modes, 10000baseCR_Full);
+			__set_bit(PHY_INTERFACE_MODE_25GBASER, interfaces);
+			__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+		} else {
+			phylink_set(modes, 10000baseCR_Full);
+			__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+		}
+	} else if (id->cable_tech & TXGBE_SFF_DA_ACTIVE_CABLE) {
+		txgbe->link_port = PORT_DA;
+		phylink_set(modes, Autoneg);
+		phylink_set(modes, 25000baseCR_Full);
 		__set_bit(PHY_INTERFACE_MODE_25GBASER, interfaces);
-	}
-	if (id->com_10g_code & TXGBE_SFF_10GBASESR_CAPABLE) {
-		phylink_set(modes, 10000baseSR_Full);
-		__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
-	}
-	if (id->com_10g_code & TXGBE_SFF_10GBASELR_CAPABLE) {
-		phylink_set(modes, 10000baseLR_Full);
-		__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+	} else {
+		if (id->com_25g_code == TXGBE_SFF_25GBASESR_CAPABLE ||
+		    id->com_25g_code == TXGBE_SFF_25GBASEER_CAPABLE ||
+		    id->com_25g_code == TXGBE_SFF_25GBASELR_CAPABLE) {
+			txgbe->link_port = PORT_FIBRE;
+			phylink_set(modes, 25000baseSR_Full);
+			__set_bit(PHY_INTERFACE_MODE_25GBASER, interfaces);
+		}
+		if (id->com_10g_code & TXGBE_SFF_10GBASESR_CAPABLE) {
+			txgbe->link_port = PORT_FIBRE;
+			phylink_set(modes, 10000baseSR_Full);
+			__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+		}
+		if (id->com_10g_code & TXGBE_SFF_10GBASELR_CAPABLE) {
+			txgbe->link_port = PORT_FIBRE;
+			phylink_set(modes, 10000baseLR_Full);
+			__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+		}
 	}
 
 	if (phy_interface_empty(interfaces)) {
@@ -177,7 +203,6 @@ static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sfp_id *id)
 	phylink_set(modes, Pause);
 	phylink_set(modes, Asym_Pause);
 	phylink_set(modes, FIBRE);
-	txgbe->link_port = PORT_FIBRE;
 
 	if (!linkmode_equal(txgbe->sfp_support, modes)) {
 		linkmode_copy(txgbe->sfp_support, modes);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
index e285b088c7b2..e8dd277a35c7 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -30,7 +30,8 @@ int txgbe_get_link_ksettings(struct net_device *netdev,
 		return 0;
 
 	cmd->base.port = txgbe->link_port;
-	cmd->base.autoneg = AUTONEG_DISABLE;
+	cmd->base.autoneg = phylink_test(txgbe->advertising, Autoneg) ?
+			    AUTONEG_ENABLE : AUTONEG_DISABLE;
 	linkmode_copy(cmd->link_modes.supported, txgbe->sfp_support);
 	linkmode_copy(cmd->link_modes.advertising, txgbe->advertising);
 
-- 
2.48.1

[PATCH net-next 2/5] net: txgbe: rename the SFP related

[Jiawen Wu]

QSFP supported will be introduced for AML 40G devices, the code related
to identify various modules should be renamed to more appropriate names.

And struct txgbe_hic_i2c_read used to get module information is renamed
as struct txgbe_hic_get_module_info, because another SW-FW command to
read I2C will be added later.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |  2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 39 ++++++++++---------
 .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |  2 +-
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    |  2 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 12 +++---
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   | 12 +++---
 6 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index b1a6ef5709a9..29e5c5470c94 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -1249,7 +1249,7 @@ enum wx_pf_flags {
 	WX_FLAG_RX_HWTSTAMP_IN_REGISTER,
 	WX_FLAG_PTP_PPS_ENABLED,
 	WX_FLAG_NEED_LINK_CONFIG,
-	WX_FLAG_NEED_SFP_RESET,
+	WX_FLAG_NEED_MODULE_RESET,
 	WX_FLAG_NEED_UPDATE_LINK,
 	WX_FLAG_NEED_DO_RESET,
 	WX_FLAG_RX_MERGE_ENABLED,
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
index a7650f548fa4..432880ccc640 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
@@ -38,7 +38,7 @@ irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data)
 	wr32(wx, WX_GPIO_INTMASK, 0xFF);
 	status = rd32(wx, WX_GPIO_INTSTATUS);
 	if (status & TXGBE_GPIOBIT_2) {
-		set_bit(WX_FLAG_NEED_SFP_RESET, wx->flags);
+		set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
 		wr32(wx, WX_GPIO_EOI, TXGBE_GPIOBIT_2);
 		wx_service_event_schedule(wx);
 	}
@@ -63,15 +63,16 @@ int txgbe_test_hostif(struct wx *wx)
 					WX_HI_COMMAND_TIMEOUT, true);
 }
 
-static int txgbe_identify_sfp_hostif(struct wx *wx, struct txgbe_hic_i2c_read *buffer)
+static int txgbe_identify_module_hostif(struct wx *wx,
+					struct txgbe_hic_get_module_info *buffer)
 {
-	buffer->hdr.cmd = FW_READ_SFP_INFO_CMD;
-	buffer->hdr.buf_len = sizeof(struct txgbe_hic_i2c_read) -
+	buffer->hdr.cmd = FW_GET_MODULE_INFO_CMD;
+	buffer->hdr.buf_len = sizeof(struct txgbe_hic_get_module_info) -
 			      sizeof(struct wx_hic_hdr);
 	buffer->hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
 
 	return wx_host_interface_command(wx, (u32 *)buffer,
-					 sizeof(struct txgbe_hic_i2c_read),
+					 sizeof(struct txgbe_hic_get_module_info),
 					 WX_HI_COMMAND_TIMEOUT, true);
 }
 
@@ -109,9 +110,9 @@ static void txgbe_get_link_capabilities(struct wx *wx, int *speed,
 {
 	struct txgbe *txgbe = wx->priv;
 
-	if (test_bit(PHY_INTERFACE_MODE_25GBASER, txgbe->sfp_interfaces))
+	if (test_bit(PHY_INTERFACE_MODE_25GBASER, txgbe->link_interfaces))
 		*speed = SPEED_25000;
-	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, txgbe->sfp_interfaces))
+	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, txgbe->link_interfaces))
 		*speed = SPEED_10000;
 	else
 		*speed = SPEED_UNKNOWN;
@@ -150,7 +151,7 @@ int txgbe_set_phy_link(struct wx *wx)
 	return 0;
 }
 
-static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sfp_id *id)
+static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sff_id *id)
 {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(modes) = { 0, };
 	DECLARE_PHY_INTERFACE_MASK(interfaces);
@@ -204,9 +205,9 @@ static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sfp_id *id)
 	phylink_set(modes, Asym_Pause);
 	phylink_set(modes, FIBRE);
 
-	if (!linkmode_equal(txgbe->sfp_support, modes)) {
-		linkmode_copy(txgbe->sfp_support, modes);
-		phy_interface_and(txgbe->sfp_interfaces,
+	if (!linkmode_equal(txgbe->link_support, modes)) {
+		linkmode_copy(txgbe->link_support, modes);
+		phy_interface_and(txgbe->link_interfaces,
 				  wx->phylink_config.supported_interfaces,
 				  interfaces);
 		linkmode_copy(txgbe->advertising, modes);
@@ -217,10 +218,10 @@ static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sfp_id *id)
 	return 0;
 }
 
-int txgbe_identify_sfp(struct wx *wx)
+int txgbe_identify_module(struct wx *wx)
 {
-	struct txgbe_hic_i2c_read buffer;
-	struct txgbe_sfp_id *id;
+	struct txgbe_hic_get_module_info buffer;
+	struct txgbe_sff_id *id;
 	int err = 0;
 	u32 gpio;
 
@@ -228,9 +229,9 @@ int txgbe_identify_sfp(struct wx *wx)
 	if (gpio & TXGBE_GPIOBIT_2)
 		return -ENODEV;
 
-	err = txgbe_identify_sfp_hostif(wx, &buffer);
+	err = txgbe_identify_module_hostif(wx, &buffer);
 	if (err) {
-		wx_err(wx, "Failed to identify SFP module\n");
+		wx_err(wx, "Failed to identify module\n");
 		return err;
 	}
 
@@ -247,10 +248,10 @@ void txgbe_setup_link(struct wx *wx)
 {
 	struct txgbe *txgbe = wx->priv;
 
-	phy_interface_zero(txgbe->sfp_interfaces);
-	linkmode_zero(txgbe->sfp_support);
+	phy_interface_zero(txgbe->link_interfaces);
+	linkmode_zero(txgbe->link_support);
 
-	txgbe_identify_sfp(wx);
+	txgbe_identify_module(wx);
 }
 
 static void txgbe_get_link_state(struct phylink_config *config,
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
index 25d4971ca0d9..7c8fa48e68d3 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
@@ -8,7 +8,7 @@ void txgbe_gpio_init_aml(struct wx *wx);
 irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data);
 int txgbe_test_hostif(struct wx *wx);
 int txgbe_set_phy_link(struct wx *wx);
-int txgbe_identify_sfp(struct wx *wx);
+int txgbe_identify_module(struct wx *wx);
 void txgbe_setup_link(struct wx *wx);
 int txgbe_phylink_init_aml(struct txgbe *txgbe);
 
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
index e8dd277a35c7..d7f905359458 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -32,7 +32,7 @@ int txgbe_get_link_ksettings(struct net_device *netdev,
 	cmd->base.port = txgbe->link_port;
 	cmd->base.autoneg = phylink_test(txgbe->advertising, Autoneg) ?
 			    AUTONEG_ENABLE : AUTONEG_DISABLE;
-	linkmode_copy(cmd->link_modes.supported, txgbe->sfp_support);
+	linkmode_copy(cmd->link_modes.supported, txgbe->link_support);
 	linkmode_copy(cmd->link_modes.advertising, txgbe->advertising);
 
 	return 0;
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index daa761e48f9d..91d1b4e68126 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -89,21 +89,21 @@ static int txgbe_enumerate_functions(struct wx *wx)
 	return physfns;
 }
 
-static void txgbe_sfp_detection_subtask(struct wx *wx)
+static void txgbe_module_detection_subtask(struct wx *wx)
 {
 	int err;
 
-	if (!test_bit(WX_FLAG_NEED_SFP_RESET, wx->flags))
+	if (!test_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags))
 		return;
 
-	/* wait for SFP module ready */
+	/* wait for SFF module ready */
 	msleep(200);
 
-	err = txgbe_identify_sfp(wx);
+	err = txgbe_identify_module(wx);
 	if (err)
 		return;
 
-	clear_bit(WX_FLAG_NEED_SFP_RESET, wx->flags);
+	clear_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
 }
 
 static void txgbe_link_config_subtask(struct wx *wx)
@@ -128,7 +128,7 @@ static void txgbe_service_task(struct work_struct *work)
 {
 	struct wx *wx = container_of(work, struct wx, service_task);
 
-	txgbe_sfp_detection_subtask(wx);
+	txgbe_module_detection_subtask(wx);
 	txgbe_link_config_subtask(wx);
 
 	wx_service_event_complete(wx);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index b9a4ba48f5b9..c115ed659544 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -341,9 +341,9 @@ void txgbe_do_reset(struct net_device *netdev);
 
 #define FW_PHY_GET_LINK_CMD             0xC0
 #define FW_PHY_SET_LINK_CMD             0xC1
-#define FW_READ_SFP_INFO_CMD            0xC5
+#define FW_GET_MODULE_INFO_CMD          0xC5
 
-struct txgbe_sfp_id {
+struct txgbe_sff_id {
 	u8 identifier;		/* A0H 0x00 */
 	u8 com_1g_code;		/* A0H 0x06 */
 	u8 com_10g_code;	/* A0H 0x03 */
@@ -358,9 +358,9 @@ struct txgbe_sfp_id {
 	u8 reserved[5];
 };
 
-struct txgbe_hic_i2c_read {
+struct txgbe_hic_get_module_info {
 	struct wx_hic_hdr hdr;
-	struct txgbe_sfp_id id;
+	struct txgbe_sff_id id;
 };
 
 struct txgbe_hic_ephy_setlink {
@@ -451,8 +451,8 @@ struct txgbe {
 	int fdir_filter_count;
 	spinlock_t fdir_perfect_lock; /* spinlock for FDIR */
 
-	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
+	DECLARE_PHY_INTERFACE_MASK(link_interfaces);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(link_support);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
 	u8 link_port;
 };
-- 
2.48.1

[PATCH net-next 3/5] net: txgbe: improve functions of AML 40G devices

[Jiawen Wu]

Support to identify QSFP modules for AML 40G devices. The definition of
GPIO pins follows the design of the QSFP modules, and TXGBE_GPIOBIT_4 is
used for module present.

Meanwhile, implement phylink in XLGMII mode by default, and get the link
state from MAC link.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 .../net/ethernet/wangxun/libwx/wx_ethtool.c   |  12 --
 .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 136 ++++++++++++++++--
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    |   3 -
 .../net/ethernet/wangxun/txgbe/txgbe_irq.c    |  10 +-
 .../net/ethernet/wangxun/txgbe/txgbe_main.c   |  11 +-
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    |   2 -
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  11 ++
 7 files changed, 137 insertions(+), 48 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 9aa3964187e1..f362e51c73ee 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -240,9 +240,6 @@ int wx_nway_reset(struct net_device *netdev)
 {
 	struct wx *wx = netdev_priv(netdev);
 
-	if (wx->mac.type == wx_mac_aml40)
-		return -EOPNOTSUPP;
-
 	return phylink_ethtool_nway_reset(wx->phylink);
 }
 EXPORT_SYMBOL(wx_nway_reset);
@@ -261,9 +258,6 @@ int wx_set_link_ksettings(struct net_device *netdev,
 {
 	struct wx *wx = netdev_priv(netdev);
 
-	if (wx->mac.type == wx_mac_aml40)
-		return -EOPNOTSUPP;
-
 	return phylink_ethtool_ksettings_set(wx->phylink, cmd);
 }
 EXPORT_SYMBOL(wx_set_link_ksettings);
@@ -273,9 +267,6 @@ void wx_get_pauseparam(struct net_device *netdev,
 {
 	struct wx *wx = netdev_priv(netdev);
 
-	if (wx->mac.type == wx_mac_aml40)
-		return;
-
 	phylink_ethtool_get_pauseparam(wx->phylink, pause);
 }
 EXPORT_SYMBOL(wx_get_pauseparam);
@@ -285,9 +276,6 @@ int wx_set_pauseparam(struct net_device *netdev,
 {
 	struct wx *wx = netdev_priv(netdev);
 
-	if (wx->mac.type == wx_mac_aml40)
-		return -EOPNOTSUPP;
-
 	return phylink_ethtool_set_pauseparam(wx->phylink, pause);
 }
 EXPORT_SYMBOL(wx_set_pauseparam);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
index 432880ccc640..5725e9557669 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
@@ -17,10 +17,15 @@
 
 void txgbe_gpio_init_aml(struct wx *wx)
 {
-	u32 status;
+	u32 status, mod_rst;
+
+	if (wx->mac.type == wx_mac_aml40)
+		mod_rst = TXGBE_GPIOBIT_4;
+	else
+		mod_rst = TXGBE_GPIOBIT_2;
 
-	wr32(wx, WX_GPIO_INTTYPE_LEVEL, TXGBE_GPIOBIT_2);
-	wr32(wx, WX_GPIO_INTEN, TXGBE_GPIOBIT_2);
+	wr32(wx, WX_GPIO_INTTYPE_LEVEL, mod_rst);
+	wr32(wx, WX_GPIO_INTEN, mod_rst);
 
 	status = rd32(wx, WX_GPIO_INTSTATUS);
 	for (int i = 0; i < 6; i++) {
@@ -33,13 +38,18 @@ irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data)
 {
 	struct txgbe *txgbe = data;
 	struct wx *wx = txgbe->wx;
-	u32 status;
+	u32 status, mod_rst;
+
+	if (wx->mac.type == wx_mac_aml40)
+		mod_rst = TXGBE_GPIOBIT_4;
+	else
+		mod_rst = TXGBE_GPIOBIT_2;
 
 	wr32(wx, WX_GPIO_INTMASK, 0xFF);
 	status = rd32(wx, WX_GPIO_INTSTATUS);
-	if (status & TXGBE_GPIOBIT_2) {
+	if (status & mod_rst) {
 		set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
-		wr32(wx, WX_GPIO_EOI, TXGBE_GPIOBIT_2);
+		wr32(wx, WX_GPIO_EOI, mod_rst);
 		wx_service_event_schedule(wx);
 	}
 
@@ -51,7 +61,7 @@ int txgbe_test_hostif(struct wx *wx)
 {
 	struct txgbe_hic_ephy_getlink buffer;
 
-	if (wx->mac.type != wx_mac_aml)
+	if (wx->mac.type == wx_mac_sp)
 		return 0;
 
 	buffer.hdr.cmd = FW_PHY_GET_LINK_CMD;
@@ -86,6 +96,9 @@ static int txgbe_set_phy_link_hostif(struct wx *wx, int speed, int autoneg, int
 	buffer.hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
 
 	switch (speed) {
+	case SPEED_40000:
+		buffer.speed = TXGBE_LINK_SPEED_40GB_FULL;
+		break;
 	case SPEED_25000:
 		buffer.speed = TXGBE_LINK_SPEED_25GB_FULL;
 		break;
@@ -110,7 +123,9 @@ static void txgbe_get_link_capabilities(struct wx *wx, int *speed,
 {
 	struct txgbe *txgbe = wx->priv;
 
-	if (test_bit(PHY_INTERFACE_MODE_25GBASER, txgbe->link_interfaces))
+	if (test_bit(PHY_INTERFACE_MODE_XLGMII, txgbe->link_interfaces))
+		*speed = SPEED_40000;
+	else if (test_bit(PHY_INTERFACE_MODE_25GBASER, txgbe->link_interfaces))
 		*speed = SPEED_25000;
 	else if (test_bit(PHY_INTERFACE_MODE_10GBASER, txgbe->link_interfaces))
 		*speed = SPEED_10000;
@@ -128,6 +143,8 @@ static void txgbe_get_mac_link(struct wx *wx, int *speed)
 	status = rd32(wx, TXGBE_CFG_PORT_ST);
 	if (!(status & TXGBE_CFG_PORT_ST_LINK_UP))
 		*speed = SPEED_UNKNOWN;
+	else if (status & TXGBE_CFG_PORT_ST_LINK_AML_40G)
+		*speed = SPEED_40000;
 	else if (status & TXGBE_CFG_PORT_ST_LINK_AML_25G)
 		*speed = SPEED_25000;
 	else if (status & TXGBE_CFG_PORT_ST_LINK_AML_10G)
@@ -218,15 +235,86 @@ static int txgbe_sfp_to_linkmodes(struct wx *wx, struct txgbe_sff_id *id)
 	return 0;
 }
 
+static int txgbe_qsfp_to_linkmodes(struct wx *wx, struct txgbe_sff_id *id)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(modes) = { 0, };
+	DECLARE_PHY_INTERFACE_MASK(interfaces);
+	struct txgbe *txgbe = wx->priv;
+
+	if (id->transceiver_type & TXGBE_SFF_ETHERNET_40G_CR4) {
+		txgbe->link_port = PORT_DA;
+		phylink_set(modes, Autoneg);
+		phylink_set(modes, 40000baseCR4_Full);
+		phylink_set(modes, 10000baseCR_Full);
+		__set_bit(PHY_INTERFACE_MODE_XLGMII, interfaces);
+		__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+	}
+	if (id->transceiver_type & TXGBE_SFF_ETHERNET_40G_SR4) {
+		txgbe->link_port = PORT_FIBRE;
+		phylink_set(modes, 40000baseSR4_Full);
+		__set_bit(PHY_INTERFACE_MODE_XLGMII, interfaces);
+	}
+	if (id->transceiver_type & TXGBE_SFF_ETHERNET_40G_LR4) {
+		txgbe->link_port = PORT_FIBRE;
+		phylink_set(modes, 40000baseLR4_Full);
+		__set_bit(PHY_INTERFACE_MODE_XLGMII, interfaces);
+	}
+	if (id->transceiver_type & TXGBE_SFF_ETHERNET_40G_ACTIVE) {
+		txgbe->link_port = PORT_DA;
+		phylink_set(modes, Autoneg);
+		phylink_set(modes, 40000baseCR4_Full);
+		__set_bit(PHY_INTERFACE_MODE_XLGMII, interfaces);
+	}
+	if (id->transceiver_type & TXGBE_SFF_ETHERNET_RSRVD) {
+		if (id->sff_opt1 & TXGBE_SFF_ETHERNET_100G_CR4) {
+			txgbe->link_port = PORT_DA;
+			phylink_set(modes, Autoneg);
+			phylink_set(modes, 40000baseCR4_Full);
+			phylink_set(modes, 25000baseCR_Full);
+			phylink_set(modes, 10000baseCR_Full);
+			__set_bit(PHY_INTERFACE_MODE_XLGMII, interfaces);
+			__set_bit(PHY_INTERFACE_MODE_25GBASER, interfaces);
+			__set_bit(PHY_INTERFACE_MODE_10GBASER, interfaces);
+		}
+	}
+
+	if (phy_interface_empty(interfaces)) {
+		wx_err(wx, "unsupported QSFP module\n");
+		return -EINVAL;
+	}
+
+	phylink_set(modes, Pause);
+	phylink_set(modes, Asym_Pause);
+	phylink_set(modes, FIBRE);
+
+	if (!linkmode_equal(txgbe->link_support, modes)) {
+		linkmode_copy(txgbe->link_support, modes);
+		phy_interface_and(txgbe->link_interfaces,
+				  wx->phylink_config.supported_interfaces,
+				  interfaces);
+		linkmode_copy(txgbe->advertising, modes);
+
+		set_bit(WX_FLAG_NEED_LINK_CONFIG, wx->flags);
+	}
+
+	return 0;
+}
+
 int txgbe_identify_module(struct wx *wx)
 {
 	struct txgbe_hic_get_module_info buffer;
 	struct txgbe_sff_id *id;
 	int err = 0;
+	u32 mod_abs;
 	u32 gpio;
 
+	if (wx->mac.type == wx_mac_aml40)
+		mod_abs = TXGBE_GPIOBIT_4;
+	else
+		mod_abs = TXGBE_GPIOBIT_2;
+
 	gpio = rd32(wx, WX_GPIO_EXT);
-	if (gpio & TXGBE_GPIOBIT_2)
+	if (gpio & mod_abs)
 		return -ENODEV;
 
 	err = txgbe_identify_module_hostif(wx, &buffer);
@@ -236,12 +324,18 @@ int txgbe_identify_module(struct wx *wx)
 	}
 
 	id = &buffer.id;
-	if (id->identifier != TXGBE_SFF_IDENTIFIER_SFP) {
-		wx_err(wx, "Invalid SFP module\n");
+	if (id->identifier != TXGBE_SFF_IDENTIFIER_SFP &&
+	    id->identifier != TXGBE_SFF_IDENTIFIER_QSFP &&
+	    id->identifier != TXGBE_SFF_IDENTIFIER_QSFP_PLUS &&
+	    id->identifier != TXGBE_SFF_IDENTIFIER_QSFP28) {
+		wx_err(wx, "Invalid module\n");
 		return -ENODEV;
 	}
 
-	return txgbe_sfp_to_linkmodes(wx, id);
+	if (id->transceiver_type == 0xFF)
+		return txgbe_sfp_to_linkmodes(wx, id);
+
+	return txgbe_qsfp_to_linkmodes(wx, id);
 }
 
 void txgbe_setup_link(struct wx *wx)
@@ -304,6 +398,9 @@ static void txgbe_mac_link_up_aml(struct phylink_config *config,
 	txcfg &= ~TXGBE_AML_MAC_TX_CFG_SPEED_MASK;
 
 	switch (speed) {
+	case SPEED_40000:
+		txcfg |= TXGBE_AML_MAC_TX_CFG_SPEED_40G;
+		break;
 	case SPEED_25000:
 		txcfg |= TXGBE_AML_MAC_TX_CFG_SPEED_25G;
 		break;
@@ -368,7 +465,18 @@ int txgbe_phylink_init_aml(struct txgbe *txgbe)
 				   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
 	config->get_fixed_state = txgbe_get_link_state;
 
-	phy_mode = PHY_INTERFACE_MODE_25GBASER;
+	if (wx->mac.type == wx_mac_aml40) {
+		config->mac_capabilities |= MAC_40000FD;
+		phy_mode = PHY_INTERFACE_MODE_XLGMII;
+		__set_bit(PHY_INTERFACE_MODE_XLGMII, config->supported_interfaces);
+		state.speed = SPEED_40000;
+		state.duplex = DUPLEX_FULL;
+	} else {
+		phy_mode = PHY_INTERFACE_MODE_25GBASER;
+		state.speed = SPEED_25000;
+		state.duplex = DUPLEX_FULL;
+	}
+
 	__set_bit(PHY_INTERFACE_MODE_25GBASER, config->supported_interfaces);
 	__set_bit(PHY_INTERFACE_MODE_10GBASER, config->supported_interfaces);
 
@@ -376,8 +484,6 @@ int txgbe_phylink_init_aml(struct txgbe *txgbe)
 	if (IS_ERR(phylink))
 		return PTR_ERR(phylink);
 
-	state.speed = SPEED_25000;
-	state.duplex = DUPLEX_FULL;
 	err = phylink_set_fixed_link(phylink, &state);
 	if (err) {
 		wx_err(wx, "Failed to set fixed link\n");
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
index d7f905359458..f553ec5f8238 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -19,9 +19,6 @@ int txgbe_get_link_ksettings(struct net_device *netdev,
 	struct txgbe *txgbe = wx->priv;
 	int err;
 
-	if (wx->mac.type == wx_mac_aml40)
-		return -EOPNOTSUPP;
-
 	err = wx_get_link_ksettings(netdev, cmd);
 	if (err)
 		return err;
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
index 3885283681ec..aa14958d439a 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_irq.c
@@ -23,7 +23,7 @@ void txgbe_irq_enable(struct wx *wx, bool queues)
 {
 	u32 misc_ien = TXGBE_PX_MISC_IEN_MASK;
 
-	if (wx->mac.type == wx_mac_aml) {
+	if (wx->mac.type != wx_mac_sp) {
 		misc_ien |= TXGBE_PX_MISC_GPIO;
 		txgbe_gpio_init_aml(wx);
 	}
@@ -201,10 +201,7 @@ static void txgbe_del_irq_domain(struct txgbe *txgbe)
 
 void txgbe_free_misc_irq(struct txgbe *txgbe)
 {
-	if (txgbe->wx->mac.type == wx_mac_aml40)
-		return;
-
-	if (txgbe->wx->mac.type == wx_mac_aml)
+	if (txgbe->wx->mac.type != wx_mac_sp)
 		free_irq(txgbe->gpio_irq, txgbe);
 
 	free_irq(txgbe->link_irq, txgbe);
@@ -219,9 +216,6 @@ int txgbe_setup_misc_irq(struct txgbe *txgbe)
 	struct wx *wx = txgbe->wx;
 	int hwirq, err;
 
-	if (wx->mac.type == wx_mac_aml40)
-		goto skip_sp_irq;
-
 	txgbe->misc.nirqs = TXGBE_IRQ_MAX;
 	txgbe->misc.domain = irq_domain_create_simple(NULL, txgbe->misc.nirqs, 0,
 						      &txgbe_misc_irq_domain_ops, txgbe);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
index 91d1b4e68126..0de051450a82 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
@@ -144,7 +144,6 @@ static void txgbe_init_service(struct wx *wx)
 static void txgbe_up_complete(struct wx *wx)
 {
 	struct net_device *netdev = wx->netdev;
-	u32 reg;
 
 	wx_control_hw(wx, true);
 	wx_configure_vectors(wx);
@@ -155,12 +154,8 @@ static void txgbe_up_complete(struct wx *wx)
 
 	switch (wx->mac.type) {
 	case wx_mac_aml40:
-		reg = rd32(wx, TXGBE_AML_MAC_TX_CFG);
-		reg &= ~TXGBE_AML_MAC_TX_CFG_SPEED_MASK;
-		reg |= TXGBE_AML_MAC_TX_CFG_SPEED_40G;
-		wr32(wx, WX_MAC_TX_CFG, reg);
-		txgbe_enable_sec_tx_path(wx);
-		netif_carrier_on(wx->netdev);
+		txgbe_setup_link(wx);
+		phylink_start(wx->phylink);
 		break;
 	case wx_mac_aml:
 		/* Enable TX laser */
@@ -276,7 +271,7 @@ void txgbe_down(struct wx *wx)
 
 	switch (wx->mac.type) {
 	case wx_mac_aml40:
-		netif_carrier_off(wx->netdev);
+		phylink_stop(wx->phylink);
 		break;
 	case wx_mac_aml:
 		phylink_stop(wx->phylink);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 03f1b9bc604d..8ea7aa07ae4e 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -579,7 +579,6 @@ int txgbe_init_phy(struct txgbe *txgbe)
 
 	switch (wx->mac.type) {
 	case wx_mac_aml40:
-		return 0;
 	case wx_mac_aml:
 		return txgbe_phylink_init_aml(txgbe);
 	case wx_mac_sp:
@@ -653,7 +652,6 @@ void txgbe_remove_phy(struct txgbe *txgbe)
 {
 	switch (txgbe->wx->mac.type) {
 	case wx_mac_aml40:
-		return;
 	case wx_mac_aml:
 		phylink_destroy(txgbe->wx->phylink);
 		return;
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index c115ed659544..e72edb9ef084 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -98,6 +98,7 @@
 /* Port cfg registers */
 #define TXGBE_CFG_PORT_ST                       0x14404
 #define TXGBE_CFG_PORT_ST_LINK_UP               BIT(0)
+#define TXGBE_CFG_PORT_ST_LINK_AML_40G          BIT(2)
 #define TXGBE_CFG_PORT_ST_LINK_AML_25G          BIT(3)
 #define TXGBE_CFG_PORT_ST_LINK_AML_10G          BIT(4)
 #define TXGBE_CFG_VXLAN                         0x14410
@@ -317,8 +318,12 @@ void txgbe_do_reset(struct net_device *netdev);
 #define TXGBE_LINK_SPEED_UNKNOWN        0
 #define TXGBE_LINK_SPEED_10GB_FULL      4
 #define TXGBE_LINK_SPEED_25GB_FULL      0x10
+#define TXGBE_LINK_SPEED_40GB_FULL      0x20
 
 #define TXGBE_SFF_IDENTIFIER_SFP        0x3
+#define TXGBE_SFF_IDENTIFIER_QSFP       0xC
+#define TXGBE_SFF_IDENTIFIER_QSFP_PLUS  0xD
+#define TXGBE_SFF_IDENTIFIER_QSFP28     0x11
 #define TXGBE_SFF_DA_PASSIVE_CABLE      0x4
 #define TXGBE_SFF_DA_ACTIVE_CABLE       0x8
 #define TXGBE_SFF_DA_SPEC_ACTIVE_LIMIT  0x4
@@ -331,6 +336,12 @@ void txgbe_do_reset(struct net_device *netdev);
 #define TXGBE_SFF_25GBASECR_91FEC       0xB
 #define TXGBE_SFF_25GBASECR_74FEC       0xC
 #define TXGBE_SFF_25GBASECR_NOFEC       0xD
+#define TXGBE_SFF_ETHERNET_RSRVD        BIT(7)
+#define TXGBE_SFF_ETHERNET_40G_CR4      BIT(3)
+#define TXGBE_SFF_ETHERNET_40G_SR4      BIT(2)
+#define TXGBE_SFF_ETHERNET_40G_LR4      BIT(1)
+#define TXGBE_SFF_ETHERNET_40G_ACTIVE   BIT(0)
+#define TXGBE_SFF_ETHERNET_100G_CR4     0xB
 
 #define TXGBE_PHY_FEC_RS                BIT(0)
 #define TXGBE_PHY_FEC_BASER             BIT(1)
-- 
2.48.1

[PATCH net-next 4/5] net: txgbe: delay to identify modules in .ndo_open

[Jiawen Wu]

For QSFP modules, there is a possibility that the module cannot be
identified when read I2C immediately in .ndo_open. So just set the flag
WX_FLAG_NEED_MODULE_RESET and do it in the subtask, which always wait
200 ms to identify the module. And this change has no impact on the
original adaptation.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
index 5725e9557669..3b6ea456fbf7 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
@@ -345,7 +345,8 @@ void txgbe_setup_link(struct wx *wx)
 	phy_interface_zero(txgbe->link_interfaces);
 	linkmode_zero(txgbe->link_support);
 
-	txgbe_identify_module(wx);
+	set_bit(WX_FLAG_NEED_MODULE_RESET, wx->flags);
+	wx_service_event_schedule(wx);
 }
 
 static void txgbe_get_link_state(struct phylink_config *config,
-- 
2.48.1

[PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Jiawen Wu]

Getting module EEPROM has been supported in TXGBE SP devices, since SFP
driver has already implemented it.

Now add support to read module EEPROM for AML devices. Towards this, add
a new firmware mailbox command to get the page data.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 37 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |  3 ++
 .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 30 +++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   | 11 ++++++
 4 files changed, 81 insertions(+)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
index 3b6ea456fbf7..12900abfa91a 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
@@ -73,6 +73,43 @@ int txgbe_test_hostif(struct wx *wx)
 					WX_HI_COMMAND_TIMEOUT, true);
 }
 
+int txgbe_read_eeprom_hostif(struct wx *wx,
+			     struct txgbe_hic_i2c_read *buffer,
+			     u32 length, u8 *data)
+{
+	u32 buf_size = sizeof(struct txgbe_hic_i2c_read) - sizeof(u8);
+	u32 total_len = buf_size + length;
+	u32 dword_len, value, i;
+	u8 local_data[256];
+	int err;
+
+	if (total_len > sizeof(local_data))
+		return -EINVAL;
+
+	buffer->hdr.cmd = FW_READ_EEPROM_CMD;
+	buffer->hdr.buf_len = sizeof(struct txgbe_hic_i2c_read) -
+			      sizeof(struct wx_hic_hdr);
+	buffer->hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
+
+	err = wx_host_interface_command(wx, (u32 *)buffer,
+					sizeof(struct txgbe_hic_i2c_read),
+					WX_HI_COMMAND_TIMEOUT, false);
+	if (err != 0)
+		return err;
+
+	dword_len = (total_len + 3) / 4;
+
+	for (i = 0; i < dword_len; i++) {
+		value = rd32a(wx, WX_FW2SW_MBOX, i);
+		le32_to_cpus(&value);
+
+		memcpy(&local_data[i * 4], &value, 4);
+	}
+
+	memcpy(data, &local_data[buf_size], length);
+	return 0;
+}
+
 static int txgbe_identify_module_hostif(struct wx *wx,
 					struct txgbe_hic_get_module_info *buffer)
 {
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
index 7c8fa48e68d3..4f6df0ee860b 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
@@ -7,6 +7,9 @@
 void txgbe_gpio_init_aml(struct wx *wx);
 irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data);
 int txgbe_test_hostif(struct wx *wx);
+int txgbe_read_eeprom_hostif(struct wx *wx,
+			     struct txgbe_hic_i2c_read *buffer,
+			     u32 length, u8 *data);
 int txgbe_set_phy_link(struct wx *wx);
 int txgbe_identify_module(struct wx *wx);
 void txgbe_setup_link(struct wx *wx);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
index f553ec5f8238..1f60121fe73c 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
@@ -10,6 +10,7 @@
 #include "../libwx/wx_lib.h"
 #include "txgbe_type.h"
 #include "txgbe_fdir.h"
+#include "txgbe_aml.h"
 #include "txgbe_ethtool.h"
 
 int txgbe_get_link_ksettings(struct net_device *netdev,
@@ -534,6 +535,34 @@ static int txgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 	return ret;
 }
 
+static int
+txgbe_get_module_eeprom_by_page(struct net_device *netdev,
+				const struct ethtool_module_eeprom *page_data,
+				struct netlink_ext_ack *extack)
+{
+	struct wx *wx = netdev_priv(netdev);
+	struct txgbe_hic_i2c_read buffer;
+	int err;
+
+	if (!test_bit(WX_FLAG_SWFW_RING, wx->flags))
+		return -EOPNOTSUPP;
+
+	buffer.length = (__force u32)cpu_to_be32(page_data->length);
+	buffer.offset = (__force u32)cpu_to_be32(page_data->offset);
+	buffer.page = page_data->page;
+	buffer.bank = page_data->bank;
+	buffer.i2c_address = page_data->i2c_address;
+
+	err = txgbe_read_eeprom_hostif(wx, &buffer, page_data->length,
+				       page_data->data);
+	if (err) {
+		wx_err(wx, "Failed to read module EEPROM\n");
+		return err;
+	}
+
+	return page_data->length;
+}
+
 static const struct ethtool_ops txgbe_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
@@ -568,6 +597,7 @@ static const struct ethtool_ops txgbe_ethtool_ops = {
 	.set_msglevel		= wx_set_msglevel,
 	.get_ts_info		= wx_get_ts_info,
 	.get_ts_stats		= wx_get_ptp_stats,
+	.get_module_eeprom_by_page	= txgbe_get_module_eeprom_by_page,
 };
 
 void txgbe_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index e72edb9ef084..3d1bec39d74c 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -353,6 +353,7 @@ void txgbe_do_reset(struct net_device *netdev);
 #define FW_PHY_GET_LINK_CMD             0xC0
 #define FW_PHY_SET_LINK_CMD             0xC1
 #define FW_GET_MODULE_INFO_CMD          0xC5
+#define FW_READ_EEPROM_CMD              0xC6
 
 struct txgbe_sff_id {
 	u8 identifier;		/* A0H 0x00 */
@@ -394,6 +395,16 @@ struct txgbe_hic_ephy_getlink {
 	u8 resv[6];
 };
 
+struct txgbe_hic_i2c_read {
+	struct wx_hic_hdr hdr;
+	u32 offset;
+	u32 length;
+	u8 page;
+	u8 bank;
+	u8 i2c_address;
+	u8 data;
+};
+
 #define NODE_PROP(_NAME, _PROP)			\
 	(const struct software_node) {		\
 		.name = _NAME,			\
-- 
2.48.1

Re: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Vadim Fedorenko]

On 12/11/2025 05:58, Jiawen Wu wrote:
> Getting module EEPROM has been supported in TXGBE SP devices, since SFP
> driver has already implemented it.
> 
> Now add support to read module EEPROM for AML devices. Towards this, add
> a new firmware mailbox command to get the page data.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 37 +++++++++++++++++++
>   .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |  3 ++
>   .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 30 +++++++++++++++
>   .../net/ethernet/wangxun/txgbe/txgbe_type.h   | 11 ++++++
>   4 files changed, 81 insertions(+)
> 
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> index 3b6ea456fbf7..12900abfa91a 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> @@ -73,6 +73,43 @@ int txgbe_test_hostif(struct wx *wx)
>   					WX_HI_COMMAND_TIMEOUT, true);
>   }
>   
> +int txgbe_read_eeprom_hostif(struct wx *wx,
> +			     struct txgbe_hic_i2c_read *buffer,
> +			     u32 length, u8 *data)
> +{
> +	u32 buf_size = sizeof(struct txgbe_hic_i2c_read) - sizeof(u8);
> +	u32 total_len = buf_size + length;
> +	u32 dword_len, value, i;
> +	u8 local_data[256];
> +	int err;
> +
> +	if (total_len > sizeof(local_data))
> +		return -EINVAL;

if it's really possible? SFF pages are 128 bytes, you reserve 256 bytes
of local buffer. What are you protecting from?

> +
> +	buffer->hdr.cmd = FW_READ_EEPROM_CMD;
> +	buffer->hdr.buf_len = sizeof(struct txgbe_hic_i2c_read) -
> +			      sizeof(struct wx_hic_hdr);
> +	buffer->hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
> +
> +	err = wx_host_interface_command(wx, (u32 *)buffer,
> +					sizeof(struct txgbe_hic_i2c_read),
> +					WX_HI_COMMAND_TIMEOUT, false);
> +	if (err != 0)
> +		return err;
> +
> +	dword_len = (total_len + 3) / 4;

round_up()?

> +
> +	for (i = 0; i < dword_len; i++) {
> +		value = rd32a(wx, WX_FW2SW_MBOX, i);
> +		le32_to_cpus(&value);
> +
> +		memcpy(&local_data[i * 4], &value, 4);
> +	}

the logic here is not clear from the first read of the code. effectively
in the reply you have the same txgbe_hic_i2c_read struct but without
data field, which is obviously VLA, but then you simply skip the result
of read of txgbe_hic_i2c_read and only provide the real data back to the
caller. Maybe you can organize the code the way it can avoid double copying?

> +
> +	memcpy(data, &local_data[buf_size], length);
> +	return 0;
> +}
> +
>   static int txgbe_identify_module_hostif(struct wx *wx,
>   					struct txgbe_hic_get_module_info *buffer)
>   {
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> index 7c8fa48e68d3..4f6df0ee860b 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> @@ -7,6 +7,9 @@
>   void txgbe_gpio_init_aml(struct wx *wx);
>   irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data);
>   int txgbe_test_hostif(struct wx *wx);
> +int txgbe_read_eeprom_hostif(struct wx *wx,
> +			     struct txgbe_hic_i2c_read *buffer,
> +			     u32 length, u8 *data);
>   int txgbe_set_phy_link(struct wx *wx);
>   int txgbe_identify_module(struct wx *wx);
>   void txgbe_setup_link(struct wx *wx);
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> index f553ec5f8238..1f60121fe73c 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> @@ -10,6 +10,7 @@
>   #include "../libwx/wx_lib.h"
>   #include "txgbe_type.h"
>   #include "txgbe_fdir.h"
> +#include "txgbe_aml.h"
>   #include "txgbe_ethtool.h"
>   
>   int txgbe_get_link_ksettings(struct net_device *netdev,
> @@ -534,6 +535,34 @@ static int txgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
>   	return ret;
>   }
>   
> +static int
> +txgbe_get_module_eeprom_by_page(struct net_device *netdev,
> +				const struct ethtool_module_eeprom *page_data,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct wx *wx = netdev_priv(netdev);
> +	struct txgbe_hic_i2c_read buffer;
> +	int err;
> +
> +	if (!test_bit(WX_FLAG_SWFW_RING, wx->flags))
> +		return -EOPNOTSUPP;
> +
> +	buffer.length = (__force u32)cpu_to_be32(page_data->length);
> +	buffer.offset = (__force u32)cpu_to_be32(page_data->offset);

maybe txgbe_hic_i2c_read::length and txgbe_hic_i2c_read::offset should
be __be32 ?

> +	buffer.page = page_data->page;
> +	buffer.bank = page_data->bank;
> +	buffer.i2c_address = page_data->i2c_address;
> +
> +	err = txgbe_read_eeprom_hostif(wx, &buffer, page_data->length,
> +				       page_data->data);
> +	if (err) {
> +		wx_err(wx, "Failed to read module EEPROM\n");
> +		return err;
> +	}
> +
> +	return page_data->length;
> +}
> +
>   static const struct ethtool_ops txgbe_ethtool_ops = {
>   	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
>   				     ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
> @@ -568,6 +597,7 @@ static const struct ethtool_ops txgbe_ethtool_ops = {
>   	.set_msglevel		= wx_set_msglevel,
>   	.get_ts_info		= wx_get_ts_info,
>   	.get_ts_stats		= wx_get_ptp_stats,
> +	.get_module_eeprom_by_page	= txgbe_get_module_eeprom_by_page,
>   };
>   
>   void txgbe_set_ethtool_ops(struct net_device *netdev)
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> index e72edb9ef084..3d1bec39d74c 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> @@ -353,6 +353,7 @@ void txgbe_do_reset(struct net_device *netdev);
>   #define FW_PHY_GET_LINK_CMD             0xC0
>   #define FW_PHY_SET_LINK_CMD             0xC1
>   #define FW_GET_MODULE_INFO_CMD          0xC5
> +#define FW_READ_EEPROM_CMD              0xC6
>   
>   struct txgbe_sff_id {
>   	u8 identifier;		/* A0H 0x00 */
> @@ -394,6 +395,16 @@ struct txgbe_hic_ephy_getlink {
>   	u8 resv[6];
>   };
>   
> +struct txgbe_hic_i2c_read {
> +	struct wx_hic_hdr hdr;
> +	u32 offset;
> +	u32 length;
> +	u8 page;
> +	u8 bank;
> +	u8 i2c_address;
> +	u8 data;
> +};
you removed struct txgbe_hic_i2c_read in the patch 2 just to introduce
it in this patch?

> +
>   #define NODE_PROP(_NAME, _PROP)			\
>   	(const struct software_node) {		\
>   		.name = _NAME,			\

RE: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Jiawen Wu]

On Wed, Nov 12, 2025 8:49 PM, Vadim Fedorenko wrote:
> On 12/11/2025 05:58, Jiawen Wu wrote:
> > Getting module EEPROM has been supported in TXGBE SP devices, since SFP
> > driver has already implemented it.
> >
> > Now add support to read module EEPROM for AML devices. Towards this, add
> > a new firmware mailbox command to get the page data.
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >   .../net/ethernet/wangxun/txgbe/txgbe_aml.c    | 37 +++++++++++++++++++
> >   .../net/ethernet/wangxun/txgbe/txgbe_aml.h    |  3 ++
> >   .../ethernet/wangxun/txgbe/txgbe_ethtool.c    | 30 +++++++++++++++
> >   .../net/ethernet/wangxun/txgbe/txgbe_type.h   | 11 ++++++
> >   4 files changed, 81 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> > index 3b6ea456fbf7..12900abfa91a 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.c
> > @@ -73,6 +73,43 @@ int txgbe_test_hostif(struct wx *wx)
> >   					WX_HI_COMMAND_TIMEOUT, true);
> >   }
> >
> > +int txgbe_read_eeprom_hostif(struct wx *wx,
> > +			     struct txgbe_hic_i2c_read *buffer,
> > +			     u32 length, u8 *data)
> > +{
> > +	u32 buf_size = sizeof(struct txgbe_hic_i2c_read) - sizeof(u8);
> > +	u32 total_len = buf_size + length;
> > +	u32 dword_len, value, i;
> > +	u8 local_data[256];
> > +	int err;
> > +
> > +	if (total_len > sizeof(local_data))
> > +		return -EINVAL;
> 
> if it's really possible? SFF pages are 128 bytes, you reserve 256 bytes
> of local buffer. What are you protecting from?

It can be changed to 128 + sizeof(struct txgbe_hic_i2c_read).

> 
> > +
> > +	buffer->hdr.cmd = FW_READ_EEPROM_CMD;
> > +	buffer->hdr.buf_len = sizeof(struct txgbe_hic_i2c_read) -
> > +			      sizeof(struct wx_hic_hdr);
> > +	buffer->hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
> > +
> > +	err = wx_host_interface_command(wx, (u32 *)buffer,
> > +					sizeof(struct txgbe_hic_i2c_read),
> > +					WX_HI_COMMAND_TIMEOUT, false);
> > +	if (err != 0)
> > +		return err;
> > +
> > +	dword_len = (total_len + 3) / 4;
> 
> round_up()?
> 
> > +
> > +	for (i = 0; i < dword_len; i++) {
> > +		value = rd32a(wx, WX_FW2SW_MBOX, i);
> > +		le32_to_cpus(&value);
> > +
> > +		memcpy(&local_data[i * 4], &value, 4);
> > +	}
> 
> the logic here is not clear from the first read of the code. effectively
> in the reply you have the same txgbe_hic_i2c_read struct but without
> data field, which is obviously VLA, but then you simply skip the result
> of read of txgbe_hic_i2c_read and only provide the real data back to the
> caller. Maybe you can organize the code the way it can avoid double copying?

Because the length of real data is variable, now it could be 1 or 128. But the total
length of the command buffer is DWORD aligned. So we designed only a 1-byte
data field in struct txgbe_hic_i2c_read, to avoid redundant reading and writing
during the SW-FW interaction.

For 1-byte data, wx_host_interface_command() can be used to set 'return_data'
to true, then page->data = buffer->data. For other cases, I think it would be more
convenient to read directly from the mailbox registers.

> 
> > +
> > +	memcpy(data, &local_data[buf_size], length);
> > +	return 0;
> > +}
> > +
> >   static int txgbe_identify_module_hostif(struct wx *wx,
> >   					struct txgbe_hic_get_module_info *buffer)
> >   {
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> > index 7c8fa48e68d3..4f6df0ee860b 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_aml.h
> > @@ -7,6 +7,9 @@
> >   void txgbe_gpio_init_aml(struct wx *wx);
> >   irqreturn_t txgbe_gpio_irq_handler_aml(int irq, void *data);
> >   int txgbe_test_hostif(struct wx *wx);
> > +int txgbe_read_eeprom_hostif(struct wx *wx,
> > +			     struct txgbe_hic_i2c_read *buffer,
> > +			     u32 length, u8 *data);
> >   int txgbe_set_phy_link(struct wx *wx);
> >   int txgbe_identify_module(struct wx *wx);
> >   void txgbe_setup_link(struct wx *wx);
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> > index f553ec5f8238..1f60121fe73c 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_ethtool.c
> > @@ -10,6 +10,7 @@
> >   #include "../libwx/wx_lib.h"
> >   #include "txgbe_type.h"
> >   #include "txgbe_fdir.h"
> > +#include "txgbe_aml.h"
> >   #include "txgbe_ethtool.h"
> >
> >   int txgbe_get_link_ksettings(struct net_device *netdev,
> > @@ -534,6 +535,34 @@ static int txgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
> >   	return ret;
> >   }
> >
> > +static int
> > +txgbe_get_module_eeprom_by_page(struct net_device *netdev,
> > +				const struct ethtool_module_eeprom *page_data,
> > +				struct netlink_ext_ack *extack)
> > +{
> > +	struct wx *wx = netdev_priv(netdev);
> > +	struct txgbe_hic_i2c_read buffer;
> > +	int err;
> > +
> > +	if (!test_bit(WX_FLAG_SWFW_RING, wx->flags))
> > +		return -EOPNOTSUPP;
> > +
> > +	buffer.length = (__force u32)cpu_to_be32(page_data->length);
> > +	buffer.offset = (__force u32)cpu_to_be32(page_data->offset);
> 
> maybe txgbe_hic_i2c_read::length and txgbe_hic_i2c_read::offset should
> be __be32 ?

Sure.

> 
> > +	buffer.page = page_data->page;
> > +	buffer.bank = page_data->bank;
> > +	buffer.i2c_address = page_data->i2c_address;
> > +
> > +	err = txgbe_read_eeprom_hostif(wx, &buffer, page_data->length,
> > +				       page_data->data);
> > +	if (err) {
> > +		wx_err(wx, "Failed to read module EEPROM\n");
> > +		return err;
> > +	}
> > +
> > +	return page_data->length;
> > +}
> > +
> >   static const struct ethtool_ops txgbe_ethtool_ops = {
> >   	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> >   				     ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ |
> > @@ -568,6 +597,7 @@ static const struct ethtool_ops txgbe_ethtool_ops = {
> >   	.set_msglevel		= wx_set_msglevel,
> >   	.get_ts_info		= wx_get_ts_info,
> >   	.get_ts_stats		= wx_get_ptp_stats,
> > +	.get_module_eeprom_by_page	= txgbe_get_module_eeprom_by_page,
> >   };
> >
> >   void txgbe_set_ethtool_ops(struct net_device *netdev)
> > diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> > index e72edb9ef084..3d1bec39d74c 100644
> > --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> > +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
> > @@ -353,6 +353,7 @@ void txgbe_do_reset(struct net_device *netdev);
> >   #define FW_PHY_GET_LINK_CMD             0xC0
> >   #define FW_PHY_SET_LINK_CMD             0xC1
> >   #define FW_GET_MODULE_INFO_CMD          0xC5
> > +#define FW_READ_EEPROM_CMD              0xC6
> >
> >   struct txgbe_sff_id {
> >   	u8 identifier;		/* A0H 0x00 */
> > @@ -394,6 +395,16 @@ struct txgbe_hic_ephy_getlink {
> >   	u8 resv[6];
> >   };
> >
> > +struct txgbe_hic_i2c_read {
> > +	struct wx_hic_hdr hdr;
> > +	u32 offset;
> > +	u32 length;
> > +	u8 page;
> > +	u8 bank;
> > +	u8 i2c_address;
> > +	u8 data;
> > +};
> you removed struct txgbe_hic_i2c_read in the patch 2 just to introduce
> it in this patch?

Yes, the name of these structure would be more appropriate.

> 
> > +
> >   #define NODE_PROP(_NAME, _PROP)			\
> >   	(const struct software_node) {		\
> >   		.name = _NAME,			\
> 
> 

Re: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Vadim Fedorenko]

On 13/11/2025 02:21, Jiawen Wu wrote:
> On Wed, Nov 12, 2025 8:49 PM, Vadim Fedorenko wrote:
>> On 12/11/2025 05:58, Jiawen Wu wrote:
>>> Getting module EEPROM has been supported in TXGBE SP devices, since SFP
>>> driver has already implemented it.
>>>
>>> Now add support to read module EEPROM for AML devices. Towards this, add
>>> a new firmware mailbox command to get the page data.
>>>
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

[...]

>>> +int txgbe_read_eeprom_hostif(struct wx *wx,
>>> +			     struct txgbe_hic_i2c_read *buffer,
>>> +			     u32 length, u8 *data)
>>> +{
>>> +	u32 buf_size = sizeof(struct txgbe_hic_i2c_read) - sizeof(u8);
>>> +	u32 total_len = buf_size + length;
>>> +	u32 dword_len, value, i;
>>> +	u8 local_data[256];
>>> +	int err;
>>> +
>>> +	if (total_len > sizeof(local_data))
>>> +		return -EINVAL;
>>
>> if it's really possible? SFF pages are 128 bytes, you reserve 256 bytes
>> of local buffer. What are you protecting from?
> 
> It can be changed to 128 + sizeof(struct txgbe_hic_i2c_read).

My point is why do you need this check at all?
It looks more like defensive programming which is discouraged in kernel.

> 
>>
>>> +
>>> +	buffer->hdr.cmd = FW_READ_EEPROM_CMD;
>>> +	buffer->hdr.buf_len = sizeof(struct txgbe_hic_i2c_read) -
>>> +			      sizeof(struct wx_hic_hdr);
>>> +	buffer->hdr.cmd_or_resp.cmd_resv = FW_CEM_CMD_RESERVED;
>>> +
>>> +	err = wx_host_interface_command(wx, (u32 *)buffer,
>>> +					sizeof(struct txgbe_hic_i2c_read),
>>> +					WX_HI_COMMAND_TIMEOUT, false);
>>> +	if (err != 0)
>>> +		return err;
>>> +
>>> +	dword_len = (total_len + 3) / 4;
>>
>> round_up()?
>>
>>> +
>>> +	for (i = 0; i < dword_len; i++) {
>>> +		value = rd32a(wx, WX_FW2SW_MBOX, i);
>>> +		le32_to_cpus(&value);
>>> +
>>> +		memcpy(&local_data[i * 4], &value, 4);
>>> +	}
>>
>> the logic here is not clear from the first read of the code. effectively
>> in the reply you have the same txgbe_hic_i2c_read struct but without
>> data field, which is obviously VLA, but then you simply skip the result
>> of read of txgbe_hic_i2c_read and only provide the real data back to the
>> caller. Maybe you can organize the code the way it can avoid double copying?
> 
> Because the length of real data is variable, now it could be 1 or 128. But the total
> length of the command buffer is DWORD aligned. So we designed only a 1-byte
> data field in struct txgbe_hic_i2c_read, to avoid redundant reading and writing
> during the SW-FW interaction.
> 
> For 1-byte data, wx_host_interface_command() can be used to set 'return_data'
> to true, then page->data = buffer->data. For other cases, I think it would be more
> convenient to read directly from the mailbox registers.

With such design you always have your return data starting at offset of
15, which is absolutely unaligned. And then it needs more buffer
dancing.

> 
>>
>>> +
>>> +	memcpy(data, &local_data[buf_size], length);
>>> +	return 0;
>>> +}
>>> +

RE: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Jiawen Wu]

> >>> +
> >>> +	for (i = 0; i < dword_len; i++) {
> >>> +		value = rd32a(wx, WX_FW2SW_MBOX, i);
> >>> +		le32_to_cpus(&value);
> >>> +
> >>> +		memcpy(&local_data[i * 4], &value, 4);
> >>> +	}
> >>
> >> the logic here is not clear from the first read of the code. effectively
> >> in the reply you have the same txgbe_hic_i2c_read struct but without
> >> data field, which is obviously VLA, but then you simply skip the result
> >> of read of txgbe_hic_i2c_read and only provide the real data back to the
> >> caller. Maybe you can organize the code the way it can avoid double copying?
> >
> > Because the length of real data is variable, now it could be 1 or 128. But the total
> > length of the command buffer is DWORD aligned. So we designed only a 1-byte
> > data field in struct txgbe_hic_i2c_read, to avoid redundant reading and writing
> > during the SW-FW interaction.
> >
> > For 1-byte data, wx_host_interface_command() can be used to set 'return_data'
> > to true, then page->data = buffer->data. For other cases, I think it would be more
> > convenient to read directly from the mailbox registers.
> 
> With such design you always have your return data starting at offset of
> 15, which is absolutely unaligned. And then it needs more buffer
> dancing.

OK. We could consider redesigning this buffer structure. Return data will start
at offset of 16, and reserve 4 bytes. And longer data will be directly read from
the mailbox register. Is this design more reasonable?

Re: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Andrew Lunn]

> For 1-byte data, wx_host_interface_command() can be used to set 'return_data'
> to true, then page->data = buffer->data. For other cases, I think it would be more
> convenient to read directly from the mailbox registers.

For 1-byte data, you need to careful where it is used. All the sensor
values are u16 and you need to read a u16 otherwise you are not
guaranteed to get consistent upper/lower bytes.

So i would not recommend 1 byte read, unless you have an SFP module
which is known to be broken, and only supports 1 byte reads.

      Andrew

RE: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Jiawen Wu]

On Fri, Nov 14, 2025 6:19 AM, Andrew Lunn wrote:
> > For 1-byte data, wx_host_interface_command() can be used to set 'return_data'
> > to true, then page->data = buffer->data. For other cases, I think it would be more
> > convenient to read directly from the mailbox registers.
> 
> For 1-byte data, you need to careful where it is used. All the sensor
> values are u16 and you need to read a u16 otherwise you are not
> guaranteed to get consistent upper/lower bytes.
> 
> So i would not recommend 1 byte read, unless you have an SFP module
> which is known to be broken, and only supports 1 byte reads.

Thanks for the reminder.

But when I use 'ethtool -m', .get_module_eeprom_by_page() is always
invoked multiple times, with 'page->length = 1' in the first time.

Re: [PATCH net-next 5/5] net: txgbe: support getting module EEPROM by page

[Andrew Lunn]

On Mon, Nov 17, 2025 at 02:14:29PM +0800, Jiawen Wu wrote:
> On Fri, Nov 14, 2025 6:19 AM, Andrew Lunn wrote:
> > > For 1-byte data, wx_host_interface_command() can be used to set 'return_data'
> > > to true, then page->data = buffer->data. For other cases, I think it would be more
> > > convenient to read directly from the mailbox registers.
> > 
> > For 1-byte data, you need to careful where it is used. All the sensor
> > values are u16 and you need to read a u16 otherwise you are not
> > guaranteed to get consistent upper/lower bytes.
> > 
> > So i would not recommend 1 byte read, unless you have an SFP module
> > which is known to be broken, and only supports 1 byte reads.
> 
> Thanks for the reminder.
> 
> But when I use 'ethtool -m', .get_module_eeprom_by_page() is always
> invoked multiple times, with 'page->length = 1' in the first time.
 
What is important is that the sensors are read using multi-byte
transfers. I expect ethtool does that correctly. You can also have
your firmware do multi-byte reads by default, with word alignment, and
then just return the byte it asked for. I believe historically user
space would always ask for a 1/2 page and then just extract what it
wanted. Getting sections smaller than a 1/2 page is new.

	Andrew