[dpdk-dev] [PATCH v1] net/axgbe: support sfp module EEPROM

[dpdk-dev] [PATCH v1] net/axgbe: support sfp module EEPROM

[asomalap]

From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>

Adding API for get_module_eeprom and get_module_info.

Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
---
 doc/guides/nics/features/axgbe.ini |   1 +
 drivers/net/axgbe/axgbe_ethdev.c   |   2 +
 drivers/net/axgbe/axgbe_phy.h      |   4 ++
 drivers/net/axgbe/axgbe_phy_impl.c | 107 +++++++++++++++++++++++++++++
 4 files changed, 114 insertions(+)

diff --git a/doc/guides/nics/features/axgbe.ini b/doc/guides/nics/features/axgbe.ini
index 0becaa097..80a3bcee1 100644
--- a/doc/guides/nics/features/axgbe.ini
+++ b/doc/guides/nics/features/axgbe.ini
@@ -18,3 +18,4 @@ Basic stats          = Y
 Linux UIO            = Y
 x86-32               = Y
 x86-64               = Y
+Module EEPROM dump   = Y
diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 867058845..ea2f9bba1 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -214,6 +214,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
 	.dev_supported_ptypes_get     = axgbe_dev_supported_ptypes_get,
 	.rx_descriptor_status         = axgbe_dev_rx_descriptor_status,
 	.tx_descriptor_status         = axgbe_dev_tx_descriptor_status,
+	.get_module_info              = axgbe_get_module_info,
+	.get_module_eeprom            = axgbe_get_module_eeprom,
 };
 
 static int axgbe_phy_reset(struct axgbe_port *pdata)
diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
index 77ee20a31..d9d7fde41 100644
--- a/drivers/net/axgbe/axgbe_phy.h
+++ b/drivers/net/axgbe/axgbe_phy.h
@@ -188,5 +188,9 @@
 #define DUPLEX_FULL             0x01
 #define DUPLEX_UNKNOWN          0xff
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo);
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				  struct rte_dev_eeprom_info *info);
 #endif
 /* PHY */
diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
index 02236ec19..53716071f 100644
--- a/drivers/net/axgbe/axgbe_phy_impl.c
+++ b/drivers/net/axgbe/axgbe_phy_impl.c
@@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
 
 #define AXGBE_SFP_EXTD_CC			31
 
+#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
+
 struct axgbe_sfp_eeprom {
 	u8 base[64];
 	u8 extd[32];
 	u8 vendor[32];
 };
 
+struct axgbe_sfp_eeprom_module {
+	u8 base[256];
+};
+
 #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
 #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
 
@@ -734,6 +740,106 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
 	return ret;
 }
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom sfp_eeprom;
+	uint8_t eeprom_addr;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		return ret;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			 &sfp_eeprom, sizeof(sfp_eeprom));
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+
+	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
+		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
+			modinfo->type = RTE_ETH_MODULE_SFF_8079;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
+		} else {
+			modinfo->type = RTE_ETH_MODULE_SFF_8472;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
+		}
+	}
+
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return 0;
+}
+
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				struct rte_dev_eeprom_info *info)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom_module sfp_eeprom;
+	uint8_t eeprom_addr;
+	uint8_t *data;
+	uint32_t i;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	if (!info || !info->length || !info->data)
+		return -EINVAL;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		return ret;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+	data = info->data;
+
+	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
+	for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i] = sfp_eeprom.base[i];
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+
+	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
+	for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = sfp_eeprom.base[i];
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return 0;
+}
+
 static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
 {
 	struct axgbe_phy_data *phy_data = pdata->phy_data;
@@ -741,6 +847,7 @@ static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
 	u8 gpio_reg, gpio_ports[2];
 	int ret;
 
+
 	/* Read the input port registers */
 	gpio_reg = 0;
 	ret = axgbe_phy_i2c_read(pdata, phy_data->sfp_gpio_address,
-- 
2.17.1

Re: [dpdk-dev] [PATCH v1] net/axgbe: support sfp module EEPROM

[Ferruh Yigit]

On 5/14/2020 2:43 PM, asomalap@amd.com wrote:
> From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> 
> Adding API for get_module_eeprom and get_module_info.
>
> Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>

Hi Amaranath,

I guess this is v3 of the patch, can you please use the version info and keep
the all versions in same thread (using --in-reply-to), this helps us to trace
the status or previous comments of the patch.

There are multiple change requests and comments to previous version, same seems
sent in this version, can you please addresss them first?
http://inbox.dpdk.org/dev/67d7272a-29fc-885f-ad8e-e865687e1267@intel.com/

> ---
>  doc/guides/nics/features/axgbe.ini |   1 +
>  drivers/net/axgbe/axgbe_ethdev.c   |   2 +
>  drivers/net/axgbe/axgbe_phy.h      |   4 ++
>  drivers/net/axgbe/axgbe_phy_impl.c | 107 +++++++++++++++++++++++++++++
>  4 files changed, 114 insertions(+)
> 
> diff --git a/doc/guides/nics/features/axgbe.ini b/doc/guides/nics/features/axgbe.ini
> index 0becaa097..80a3bcee1 100644
> --- a/doc/guides/nics/features/axgbe.ini
> +++ b/doc/guides/nics/features/axgbe.ini
> @@ -18,3 +18,4 @@ Basic stats          = Y
>  Linux UIO            = Y
>  x86-32               = Y
>  x86-64               = Y
> +Module EEPROM dump   = Y

Can you please add the items with same order as 'default.ini'

[dpdk-dev] [PATCH v4] net/axgbe: support sfp module EEPROM

[asomalap]

From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>

Adding API for get_module_eeprom and get_module_info.wq

Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
---
 doc/guides/nics/features/axgbe.ini |   1 +
 drivers/net/axgbe/axgbe_ethdev.c   |   2 +
 drivers/net/axgbe/axgbe_phy.h      |   4 ++
 drivers/net/axgbe/axgbe_phy_impl.c | 108 +++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+)

diff --git a/doc/guides/nics/features/axgbe.ini b/doc/guides/nics/features/axgbe.ini
index 34df0d1ee..3a090183e 100644
--- a/doc/guides/nics/features/axgbe.ini
+++ b/doc/guides/nics/features/axgbe.ini
@@ -17,6 +17,7 @@ CRC offload          = Y
 L3 checksum offload  = Y
 L4 checksum offload  = Y
 Basic stats          = Y
+Module EEPROM dump   = Y
 Linux UIO            = Y
 x86-32               = Y
 x86-64               = Y
diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index cfe6aba73..22e05e8cb 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -257,6 +257,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
 	.timesync_adjust_time         = axgbe_timesync_adjust_time,
 	.timesync_read_time           = axgbe_timesync_read_time,
 	.timesync_write_time          = axgbe_timesync_write_time,
+	.get_module_info              = axgbe_get_module_info,
+	.get_module_eeprom            = axgbe_get_module_eeprom,
 };
 
 static int axgbe_phy_reset(struct axgbe_port *pdata)
diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
index 77ee20a31..d9d7fde41 100644
--- a/drivers/net/axgbe/axgbe_phy.h
+++ b/drivers/net/axgbe/axgbe_phy.h
@@ -188,5 +188,9 @@
 #define DUPLEX_FULL             0x01
 #define DUPLEX_UNKNOWN          0xff
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo);
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				  struct rte_dev_eeprom_info *info);
 #endif
 /* PHY */
diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
index 02236ec19..af18a8d51 100644
--- a/drivers/net/axgbe/axgbe_phy_impl.c
+++ b/drivers/net/axgbe/axgbe_phy_impl.c
@@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
 
 #define AXGBE_SFP_EXTD_CC			31
 
+#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
+
 struct axgbe_sfp_eeprom {
 	u8 base[64];
 	u8 extd[32];
 	u8 vendor[32];
 };
 
+struct axgbe_sfp_eeprom_module {
+	u8 base[AXGBE_SFP_EEPROM_PAGE_SIZE];
+};
+
 #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
 #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
 
@@ -734,6 +740,108 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
 	return ret;
 }
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom sfp_eeprom;
+	uint8_t eeprom_addr;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		goto put;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			 &sfp_eeprom, sizeof(sfp_eeprom));
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+
+	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
+		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
+			modinfo->type = RTE_ETH_MODULE_SFF_8079;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
+		} else {
+			modinfo->type = RTE_ETH_MODULE_SFF_8472;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
+		}
+	}
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return ret;
+}
+
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				struct rte_dev_eeprom_info *info)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom_module sfp_eeprom;
+	uint8_t eeprom_addr;
+	uint8_t *data;
+	uint32_t i;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	if (!info || !info->length || !info->data) {
+		axgbe_phy_put_comm_ownership(pdata);
+		return -EINVAL;
+	}
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		goto put;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+	data = info->data;
+
+	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
+	for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i] = sfp_eeprom.base[i];
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+
+	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
+	for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = sfp_eeprom.base[i];
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return ret;
+}
+
+
 static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
 {
 	struct axgbe_phy_data *phy_data = pdata->phy_data;
-- 
2.25.1

Re: [dpdk-dev] [PATCH v4] net/axgbe: support sfp module EEPROM

[Ferruh Yigit]

On 12/22/2020 7:01 AM, asomalap@amd.com wrote:
> From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> 
> Adding API for get_module_eeprom and get_module_info.wq
> 
> Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> ---
>   doc/guides/nics/features/axgbe.ini |   1 +
>   drivers/net/axgbe/axgbe_ethdev.c   |   2 +
>   drivers/net/axgbe/axgbe_phy.h      |   4 ++
>   drivers/net/axgbe/axgbe_phy_impl.c | 108 +++++++++++++++++++++++++++++
>   4 files changed, 115 insertions(+)
> 
> diff --git a/doc/guides/nics/features/axgbe.ini b/doc/guides/nics/features/axgbe.ini
> index 34df0d1ee..3a090183e 100644
> --- a/doc/guides/nics/features/axgbe.ini
> +++ b/doc/guides/nics/features/axgbe.ini
> @@ -17,6 +17,7 @@ CRC offload          = Y
>   L3 checksum offload  = Y
>   L4 checksum offload  = Y
>   Basic stats          = Y
> +Module EEPROM dump   = Y
>   Linux UIO            = Y
>   x86-32               = Y
>   x86-64               = Y
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index cfe6aba73..22e05e8cb 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -257,6 +257,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
>   	.timesync_adjust_time         = axgbe_timesync_adjust_time,
>   	.timesync_read_time           = axgbe_timesync_read_time,
>   	.timesync_write_time          = axgbe_timesync_write_time,
> +	.get_module_info              = axgbe_get_module_info,
> +	.get_module_eeprom            = axgbe_get_module_eeprom,
>   };
>   
>   static int axgbe_phy_reset(struct axgbe_port *pdata)
> diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
> index 77ee20a31..d9d7fde41 100644
> --- a/drivers/net/axgbe/axgbe_phy.h
> +++ b/drivers/net/axgbe/axgbe_phy.h
> @@ -188,5 +188,9 @@
>   #define DUPLEX_FULL             0x01
>   #define DUPLEX_UNKNOWN          0xff
>   
> +int axgbe_get_module_info(struct rte_eth_dev *dev,
> +				struct rte_eth_dev_module_info *modinfo);
> +int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
> +				  struct rte_dev_eeprom_info *info);
>   #endif
>   /* PHY */
> diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
> index 02236ec19..af18a8d51 100644
> --- a/drivers/net/axgbe/axgbe_phy_impl.c
> +++ b/drivers/net/axgbe/axgbe_phy_impl.c
> @@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
>   
>   #define AXGBE_SFP_EXTD_CC			31
>   
> +#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
> +
>   struct axgbe_sfp_eeprom {
>   	u8 base[64];
>   	u8 extd[32];
>   	u8 vendor[32];
>   };
>   
> +struct axgbe_sfp_eeprom_module {
> +	u8 base[AXGBE_SFP_EEPROM_PAGE_SIZE];
> +};
> +
>   #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
>   #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
>   
> @@ -734,6 +740,108 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
>   	return ret;
>   }
>   
> +int axgbe_get_module_info(struct rte_eth_dev *dev,
> +				struct rte_eth_dev_module_info *modinfo)
> +{
> +	struct axgbe_port *pdata = dev->data->dev_private;
> +	struct axgbe_sfp_eeprom sfp_eeprom;
> +	uint8_t eeprom_addr;
> +	int ret;
> +
> +	ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +	if (ret)
> +		return -EIO;
> +
> +	ret = axgbe_phy_sfp_get_mux(pdata);
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +		goto put;
> +	}
> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			 &sfp_eeprom, sizeof(sfp_eeprom));
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +		goto put;
> +	}
> +
> +	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {

The 'else' case of this check seems wrong, which returns success but doesn't set 
modinfo correctly.

> +		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
> +			modinfo->type = RTE_ETH_MODULE_SFF_8079;
> +			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
> +		} else {
> +			modinfo->type = RTE_ETH_MODULE_SFF_8472;
> +			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
> +		}
> +	}
> +
> +put:
> +	axgbe_phy_sfp_put_mux(pdata);
> +	axgbe_phy_put_comm_ownership(pdata);
> +	return ret;
> +}
> +
> +int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
> +				struct rte_dev_eeprom_info *info)
> +{
> +	struct axgbe_port *pdata = dev->data->dev_private;
> +	struct axgbe_sfp_eeprom_module sfp_eeprom;
> +	uint8_t eeprom_addr;
> +	uint8_t *data;
> +	uint32_t i;
> +	int ret;
> +
> +	ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +	if (ret)
> +		return -EIO;
> +
> +	if (!info || !info->length || !info->data) {

Up to you but it may be easier to verify the input and fail before taking the 
ownership of the device first.

> +		axgbe_phy_put_comm_ownership(pdata);
> +		return -EINVAL;
> +	}
> +
> +	ret = axgbe_phy_sfp_get_mux(pdata);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +		goto put;
> +	}

Just a syntax issue, some lines has an empty line between 'ret' assignment and 
'ret' check, but some does not (as above), can you please make it consistent.

> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			&sfp_eeprom, sizeof(sfp_eeprom));
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +		goto put;
> +	}
> +	data = info->data;
> +
> +	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
> +	for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +		data[i] = sfp_eeprom.base[i];

Should check if 'info->length' > 'AXGBE_SFP_SERIAL_ID_ADDRESS' before above 
assignment.

> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			&sfp_eeprom, sizeof(sfp_eeprom));
> +
> +	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
> +	for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +		data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = sfp_eeprom.base[i];
> +

Will it be acceptable if the loop break in the middle because of missing space, 
"i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE"? If not, size needs to be 
checked before the loop.

> +put:
> +	axgbe_phy_sfp_put_mux(pdata);
> +	axgbe_phy_put_comm_ownership(pdata);
> +	return ret;
> +}
> +
> +
>   static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
>   {
>   	struct axgbe_phy_data *phy_data = pdata->phy_data;
> 

Re: [dpdk-dev] [PATCH v4] net/axgbe: support sfp module EEPROM

[Somalapuram, Amaranath]

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Monday, January 4, 2021 7:54 PM
To: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>; dev@dpdk.org
Subject: Re: [PATCH v4] net/axgbe: support sfp module EEPROM

[CAUTION: External Email]

On 12/22/2020 7:01 AM, asomalap@amd.com wrote:
> From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
>
> Adding API for get_module_eeprom and get_module_info.wq
>
> Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> ---
>   doc/guides/nics/features/axgbe.ini |   1 +
>   drivers/net/axgbe/axgbe_ethdev.c   |   2 +
>   drivers/net/axgbe/axgbe_phy.h      |   4 ++
>   drivers/net/axgbe/axgbe_phy_impl.c | 108 +++++++++++++++++++++++++++++
>   4 files changed, 115 insertions(+)
>
> diff --git a/doc/guides/nics/features/axgbe.ini 
> b/doc/guides/nics/features/axgbe.ini
> index 34df0d1ee..3a090183e 100644
> --- a/doc/guides/nics/features/axgbe.ini
> +++ b/doc/guides/nics/features/axgbe.ini
> @@ -17,6 +17,7 @@ CRC offload          = Y
>   L3 checksum offload  = Y
>   L4 checksum offload  = Y
>   Basic stats          = Y
> +Module EEPROM dump   = Y
>   Linux UIO            = Y
>   x86-32               = Y
>   x86-64               = Y
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c 
> b/drivers/net/axgbe/axgbe_ethdev.c
> index cfe6aba73..22e05e8cb 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -257,6 +257,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
>       .timesync_adjust_time         = axgbe_timesync_adjust_time,
>       .timesync_read_time           = axgbe_timesync_read_time,
>       .timesync_write_time          = axgbe_timesync_write_time,
> +     .get_module_info              = axgbe_get_module_info,
> +     .get_module_eeprom            = axgbe_get_module_eeprom,
>   };
>
>   static int axgbe_phy_reset(struct axgbe_port *pdata) diff --git 
> a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h index 
> 77ee20a31..d9d7fde41 100644
> --- a/drivers/net/axgbe/axgbe_phy.h
> +++ b/drivers/net/axgbe/axgbe_phy.h
> @@ -188,5 +188,9 @@
>   #define DUPLEX_FULL             0x01
>   #define DUPLEX_UNKNOWN          0xff
>
> +int axgbe_get_module_info(struct rte_eth_dev *dev,
> +                             struct rte_eth_dev_module_info 
> +*modinfo); int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
> +                               struct rte_dev_eeprom_info *info);
>   #endif
>   /* PHY */
> diff --git a/drivers/net/axgbe/axgbe_phy_impl.c 
> b/drivers/net/axgbe/axgbe_phy_impl.c
> index 02236ec19..af18a8d51 100644
> --- a/drivers/net/axgbe/axgbe_phy_impl.c
> +++ b/drivers/net/axgbe/axgbe_phy_impl.c
> @@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
>
>   #define AXGBE_SFP_EXTD_CC                   31
>
> +#define AXGBE_SFP_EEPROM_PAGE_SIZE           256
> +
>   struct axgbe_sfp_eeprom {
>       u8 base[64];
>       u8 extd[32];
>       u8 vendor[32];
>   };
>
> +struct axgbe_sfp_eeprom_module {
> +     u8 base[AXGBE_SFP_EEPROM_PAGE_SIZE]; };
> +
>   #define AXGBE_BEL_FUSE_VENDOR       "BEL-FUSE"
>   #define AXGBE_BEL_FUSE_PARTNO       "1GBT-SFP06"
>
> @@ -734,6 +740,108 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
>       return ret;
>   }
>
> +int axgbe_get_module_info(struct rte_eth_dev *dev,
> +                             struct rte_eth_dev_module_info *modinfo) 
> +{
> +     struct axgbe_port *pdata = dev->data->dev_private;
> +     struct axgbe_sfp_eeprom sfp_eeprom;
> +     uint8_t eeprom_addr;
> +     int ret;
> +
> +     ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +     if (ret)
> +             return -EIO;
> +
> +     ret = axgbe_phy_sfp_get_mux(pdata);
> +
> +     if (ret) {
> +             PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +             goto put;
> +     }
> +
> +     eeprom_addr = 0;
> +     ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +                     &eeprom_addr, sizeof(eeprom_addr),
> +                      &sfp_eeprom, sizeof(sfp_eeprom));
> +
> +     if (ret) {
> +             PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +             goto put;
> +     }
> +
> +     if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {

The 'else' case of this check seems wrong, which returns success but doesn't set modinfo correctly.

I have used the reference code from i40e_ethdev.c and ixgbe_ethdev.c. let me know your inputs.

> +             if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
> +                     modinfo->type = RTE_ETH_MODULE_SFF_8079;
> +                     modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
> +             } else {
> +                     modinfo->type = RTE_ETH_MODULE_SFF_8472;
> +                     modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
> +             }
> +     }
> +
> +put:
> +     axgbe_phy_sfp_put_mux(pdata);
> +     axgbe_phy_put_comm_ownership(pdata);
> +     return ret;
> +}
> +
> +int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
> +                             struct rte_dev_eeprom_info *info) {
> +     struct axgbe_port *pdata = dev->data->dev_private;
> +     struct axgbe_sfp_eeprom_module sfp_eeprom;
> +     uint8_t eeprom_addr;
> +     uint8_t *data;
> +     uint32_t i;
> +     int ret;
> +
> +     ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +     if (ret)
> +             return -EIO;
> +
> +     if (!info || !info->length || !info->data) {

Up to you but it may be easier to verify the input and fail before taking the ownership of the device first.
Agreed.
> +             axgbe_phy_put_comm_ownership(pdata);
> +             return -EINVAL;
> +     }
> +
> +     ret = axgbe_phy_sfp_get_mux(pdata);
> +     if (ret) {
> +             PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +             goto put;
> +     }

Just a syntax issue, some lines has an empty line between 'ret' assignment and 'ret' check, but some does not (as above), can you please make it consistent.
Agreed.
> +
> +     eeprom_addr = 0;
> +     ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +                     &eeprom_addr, sizeof(eeprom_addr),
> +                     &sfp_eeprom, sizeof(sfp_eeprom));
> +     if (ret) {
> +             PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +             goto put;
> +     }
> +     data = info->data;
> +
> +     /* for AXGBE_SFP_SERIAL_ID_ADDRESS */
> +     for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +             data[i] = sfp_eeprom.base[i];

Should check if 'info->length' > 'AXGBE_SFP_SERIAL_ID_ADDRESS' before above assignment.
Agreed.
> +
> +     eeprom_addr = 0;
> +     ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
> +                     &eeprom_addr, sizeof(eeprom_addr),
> +                     &sfp_eeprom, sizeof(sfp_eeprom));
> +
> +     /* for AXGBE_SFP_DIAG_INFO_ADDRESS */
> +     for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +             data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = 
> + sfp_eeprom.base[i];
> +

Will it be acceptable if the loop break in the middle because of missing space, "i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE"? If not, size needs to be checked before the loop.
Agreed.
> +put:
> +     axgbe_phy_sfp_put_mux(pdata);
> +     axgbe_phy_put_comm_ownership(pdata);
> +     return ret;
> +}
> +
> +
>   static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
>   {
>       struct axgbe_phy_data *phy_data = pdata->phy_data;
>

Please reply for the else case issue. I can resubmit. Other I will fix.

Re: [dpdk-dev] [PATCH v4] net/axgbe: support sfp module EEPROM

[Ferruh Yigit]

On 1/6/2021 12:31 PM, Somalapuram, Amaranath wrote:
> 
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, January 4, 2021 7:54 PM
> To: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com>; dev@dpdk.org
> Subject: Re: [PATCH v4] net/axgbe: support sfp module EEPROM
> 
> [CAUTION: External Email]
> 
> On 12/22/2020 7:01 AM, asomalap@amd.com wrote:
>> From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
>>
>> Adding API for get_module_eeprom and get_module_info.wq
>>
>> Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
>> ---
>>    doc/guides/nics/features/axgbe.ini |   1 +
>>    drivers/net/axgbe/axgbe_ethdev.c   |   2 +
>>    drivers/net/axgbe/axgbe_phy.h      |   4 ++
>>    drivers/net/axgbe/axgbe_phy_impl.c | 108 +++++++++++++++++++++++++++++
>>    4 files changed, 115 insertions(+)
>>
>> diff --git a/doc/guides/nics/features/axgbe.ini
>> b/doc/guides/nics/features/axgbe.ini
>> index 34df0d1ee..3a090183e 100644
>> --- a/doc/guides/nics/features/axgbe.ini
>> +++ b/doc/guides/nics/features/axgbe.ini
>> @@ -17,6 +17,7 @@ CRC offload          = Y
>>    L3 checksum offload  = Y
>>    L4 checksum offload  = Y
>>    Basic stats          = Y
>> +Module EEPROM dump   = Y
>>    Linux UIO            = Y
>>    x86-32               = Y
>>    x86-64               = Y
>> diff --git a/drivers/net/axgbe/axgbe_ethdev.c
>> b/drivers/net/axgbe/axgbe_ethdev.c
>> index cfe6aba73..22e05e8cb 100644
>> --- a/drivers/net/axgbe/axgbe_ethdev.c
>> +++ b/drivers/net/axgbe/axgbe_ethdev.c
>> @@ -257,6 +257,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
>>        .timesync_adjust_time         = axgbe_timesync_adjust_time,
>>        .timesync_read_time           = axgbe_timesync_read_time,
>>        .timesync_write_time          = axgbe_timesync_write_time,
>> +     .get_module_info              = axgbe_get_module_info,
>> +     .get_module_eeprom            = axgbe_get_module_eeprom,
>>    };
>>
>>    static int axgbe_phy_reset(struct axgbe_port *pdata) diff --git
>> a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h index
>> 77ee20a31..d9d7fde41 100644
>> --- a/drivers/net/axgbe/axgbe_phy.h
>> +++ b/drivers/net/axgbe/axgbe_phy.h
>> @@ -188,5 +188,9 @@
>>    #define DUPLEX_FULL             0x01
>>    #define DUPLEX_UNKNOWN          0xff
>>
>> +int axgbe_get_module_info(struct rte_eth_dev *dev,
>> +                             struct rte_eth_dev_module_info
>> +*modinfo); int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
>> +                               struct rte_dev_eeprom_info *info);
>>    #endif
>>    /* PHY */
>> diff --git a/drivers/net/axgbe/axgbe_phy_impl.c
>> b/drivers/net/axgbe/axgbe_phy_impl.c
>> index 02236ec19..af18a8d51 100644
>> --- a/drivers/net/axgbe/axgbe_phy_impl.c
>> +++ b/drivers/net/axgbe/axgbe_phy_impl.c
>> @@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
>>
>>    #define AXGBE_SFP_EXTD_CC                   31
>>
>> +#define AXGBE_SFP_EEPROM_PAGE_SIZE           256
>> +
>>    struct axgbe_sfp_eeprom {
>>        u8 base[64];
>>        u8 extd[32];
>>        u8 vendor[32];
>>    };
>>
>> +struct axgbe_sfp_eeprom_module {
>> +     u8 base[AXGBE_SFP_EEPROM_PAGE_SIZE]; };
>> +
>>    #define AXGBE_BEL_FUSE_VENDOR       "BEL-FUSE"
>>    #define AXGBE_BEL_FUSE_PARTNO       "1GBT-SFP06"
>>
>> @@ -734,6 +740,108 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
>>        return ret;
>>    }
>>
>> +int axgbe_get_module_info(struct rte_eth_dev *dev,
>> +                             struct rte_eth_dev_module_info *modinfo)
>> +{
>> +     struct axgbe_port *pdata = dev->data->dev_private;
>> +     struct axgbe_sfp_eeprom sfp_eeprom;
>> +     uint8_t eeprom_addr;
>> +     int ret;
>> +
>> +     ret = axgbe_phy_get_comm_ownership(pdata);
>> +
>> +     if (ret)
>> +             return -EIO;
>> +
>> +     ret = axgbe_phy_sfp_get_mux(pdata);
>> +
>> +     if (ret) {
>> +             PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
>> +             goto put;
>> +     }
>> +
>> +     eeprom_addr = 0;
>> +     ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
>> +                     &eeprom_addr, sizeof(eeprom_addr),
>> +                      &sfp_eeprom, sizeof(sfp_eeprom));
>> +
>> +     if (ret) {
>> +             PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
>> +             goto put;
>> +     }
>> +
>> +     if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
> 
> The 'else' case of this check seems wrong, which returns success but doesn't set modinfo correctly.
> 
> I have used the reference code from i40e_ethdev.c and ixgbe_ethdev.c. let me know your inputs.
> 

I am not sure what exactly you are referencing in i40e & ixgbe, as far as I can 
see there is no similar usage on them, if there is please show it to me.

Above issue is, what if "sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0xff", what 
will happen in that case?
As far as I can see in that case function returns success, but the 'modinfo' is 
not updated at all, am I missing something?

>> +             if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
>> +                     modinfo->type = RTE_ETH_MODULE_SFF_8079;
>> +                     modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
>> +             } else {
>> +                     modinfo->type = RTE_ETH_MODULE_SFF_8472;
>> +                     modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
>> +             }
>> +     }
>> +
>> +put:
>> +     axgbe_phy_sfp_put_mux(pdata);
>> +     axgbe_phy_put_comm_ownership(pdata);
>> +     return ret;
>> +}
>> +

<...>

[dpdk-dev] [PATCH v5] net/axgbe: support sfp module EEPROM

[asomalap]

From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>

Adding API for get_module_eeprom and get_module_info.

Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
---
 doc/guides/nics/features/axgbe.ini |   1 +
 drivers/net/axgbe/axgbe_ethdev.c   |   2 +
 drivers/net/axgbe/axgbe_phy.h      |   4 +
 drivers/net/axgbe/axgbe_phy_impl.c | 114 +++++++++++++++++++++++++++++
 4 files changed, 121 insertions(+)

diff --git a/doc/guides/nics/features/axgbe.ini b/doc/guides/nics/features/axgbe.ini
index 34df0d1ee..3a090183e 100644
--- a/doc/guides/nics/features/axgbe.ini
+++ b/doc/guides/nics/features/axgbe.ini
@@ -17,6 +17,7 @@ CRC offload          = Y
 L3 checksum offload  = Y
 L4 checksum offload  = Y
 Basic stats          = Y
+Module EEPROM dump   = Y
 Linux UIO            = Y
 x86-32               = Y
 x86-64               = Y
diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index cfe6aba73..22e05e8cb 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -257,6 +257,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
 	.timesync_adjust_time         = axgbe_timesync_adjust_time,
 	.timesync_read_time           = axgbe_timesync_read_time,
 	.timesync_write_time          = axgbe_timesync_write_time,
+	.get_module_info              = axgbe_get_module_info,
+	.get_module_eeprom            = axgbe_get_module_eeprom,
 };
 
 static int axgbe_phy_reset(struct axgbe_port *pdata)
diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
index 77ee20a31..d9d7fde41 100644
--- a/drivers/net/axgbe/axgbe_phy.h
+++ b/drivers/net/axgbe/axgbe_phy.h
@@ -188,5 +188,9 @@
 #define DUPLEX_FULL             0x01
 #define DUPLEX_UNKNOWN          0xff
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo);
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				  struct rte_dev_eeprom_info *info);
 #endif
 /* PHY */
diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
index 02236ec19..e60f51ede 100644
--- a/drivers/net/axgbe/axgbe_phy_impl.c
+++ b/drivers/net/axgbe/axgbe_phy_impl.c
@@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
 
 #define AXGBE_SFP_EXTD_CC			31
 
+#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
+
 struct axgbe_sfp_eeprom {
 	u8 base[64];
 	u8 extd[32];
 	u8 vendor[32];
 };
 
+struct axgbe_sfp_eeprom_module {
+	u8 base[AXGBE_SFP_EEPROM_PAGE_SIZE];
+};
+
 #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
 #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
 
@@ -734,6 +740,114 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
 	return ret;
 }
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom sfp_eeprom;
+	uint8_t eeprom_addr;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		goto put;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			 &sfp_eeprom, sizeof(sfp_eeprom));
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+
+	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
+		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
+			modinfo->type = RTE_ETH_MODULE_SFF_8079;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
+		} else {
+			ret = -EIO;
+			goto put;
+		}
+	}
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return ret;
+}
+
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				struct rte_dev_eeprom_info *info)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom_module sfp_eeprom;
+	uint8_t eeprom_addr;
+	uint8_t *data;
+	uint32_t i;
+	int ret;
+
+	if (!info || !info->length || !info->data) {
+		axgbe_phy_put_comm_ownership(pdata);
+		return -EINVAL;
+	}
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		goto put;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+
+	data = info->data;
+
+	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
+	if (info->length > AXGBE_SFP_EEPROM_PAGE_SIZE)
+		for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+			data[i] = sfp_eeprom.base[i];
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+
+	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
+	if (info->length > AXGBE_SFP_EEPROM_PAGE_SIZE)
+		for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+			data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] =
+							sfp_eeprom.base[i];
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return ret;
+}
+
+
 static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
 {
 	struct axgbe_phy_data *phy_data = pdata->phy_data;
-- 
2.25.1

Re: [dpdk-dev] [PATCH v5] net/axgbe: support sfp module EEPROM

[Ferruh Yigit]

On 1/12/2021 5:28 AM, asomalap@amd.com wrote:
> From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> 
> Adding API for get_module_eeprom and get_module_info.
> 
> Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> ---
>   doc/guides/nics/features/axgbe.ini |   1 +
>   drivers/net/axgbe/axgbe_ethdev.c   |   2 +
>   drivers/net/axgbe/axgbe_phy.h      |   4 +
>   drivers/net/axgbe/axgbe_phy_impl.c | 114 +++++++++++++++++++++++++++++
>   4 files changed, 121 insertions(+)
> 
> diff --git a/doc/guides/nics/features/axgbe.ini b/doc/guides/nics/features/axgbe.ini
> index 34df0d1ee..3a090183e 100644
> --- a/doc/guides/nics/features/axgbe.ini
> +++ b/doc/guides/nics/features/axgbe.ini
> @@ -17,6 +17,7 @@ CRC offload          = Y
>   L3 checksum offload  = Y
>   L4 checksum offload  = Y
>   Basic stats          = Y
> +Module EEPROM dump   = Y
>   Linux UIO            = Y
>   x86-32               = Y
>   x86-64               = Y
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index cfe6aba73..22e05e8cb 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -257,6 +257,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
>   	.timesync_adjust_time         = axgbe_timesync_adjust_time,
>   	.timesync_read_time           = axgbe_timesync_read_time,
>   	.timesync_write_time          = axgbe_timesync_write_time,
> +	.get_module_info              = axgbe_get_module_info,
> +	.get_module_eeprom            = axgbe_get_module_eeprom,
>   };
>   
>   static int axgbe_phy_reset(struct axgbe_port *pdata)
> diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
> index 77ee20a31..d9d7fde41 100644
> --- a/drivers/net/axgbe/axgbe_phy.h
> +++ b/drivers/net/axgbe/axgbe_phy.h
> @@ -188,5 +188,9 @@
>   #define DUPLEX_FULL             0x01
>   #define DUPLEX_UNKNOWN          0xff
>   
> +int axgbe_get_module_info(struct rte_eth_dev *dev,
> +				struct rte_eth_dev_module_info *modinfo);
> +int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
> +				  struct rte_dev_eeprom_info *info);
>   #endif
>   /* PHY */
> diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
> index 02236ec19..e60f51ede 100644
> --- a/drivers/net/axgbe/axgbe_phy_impl.c
> +++ b/drivers/net/axgbe/axgbe_phy_impl.c
> @@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
>   
>   #define AXGBE_SFP_EXTD_CC			31
>   
> +#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
> +
>   struct axgbe_sfp_eeprom {
>   	u8 base[64];
>   	u8 extd[32];
>   	u8 vendor[32];
>   };
>   
> +struct axgbe_sfp_eeprom_module {
> +	u8 base[AXGBE_SFP_EEPROM_PAGE_SIZE];
> +};
> +
>   #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
>   #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
>   
> @@ -734,6 +740,114 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
>   	return ret;
>   }
>   
> +int axgbe_get_module_info(struct rte_eth_dev *dev,
> +				struct rte_eth_dev_module_info *modinfo)
> +{
> +	struct axgbe_port *pdata = dev->data->dev_private;
> +	struct axgbe_sfp_eeprom sfp_eeprom;
> +	uint8_t eeprom_addr;
> +	int ret;
> +
> +	ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +	if (ret)
> +		return -EIO;
> +
> +	ret = axgbe_phy_sfp_get_mux(pdata);
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +		goto put;
> +	}
> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			 &sfp_eeprom, sizeof(sfp_eeprom));
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +		goto put;
> +	}
> +
> +	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
> +		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
> +			modinfo->type = RTE_ETH_MODULE_SFF_8079;
> +			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
> +		} else {
> +			ret = -EIO;
> +			goto put;
> +		}
> +	}

The issue mentioned is still valid, can you please check comment in the previous 
version(s).

> +
> +put:
> +	axgbe_phy_sfp_put_mux(pdata);
> +	axgbe_phy_put_comm_ownership(pdata);
> +	return ret;
> +}
> +
> +int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
> +				struct rte_dev_eeprom_info *info)
> +{
> +	struct axgbe_port *pdata = dev->data->dev_private;
> +	struct axgbe_sfp_eeprom_module sfp_eeprom;
> +	uint8_t eeprom_addr;
> +	uint8_t *data;
> +	uint32_t i;
> +	int ret;
> +
> +	if (!info || !info->length || !info->data) {
> +		axgbe_phy_put_comm_ownership(pdata);
> +		return -EINVAL;
> +	}
> +
> +	ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +	if (ret)
> +		return -EIO;
> +
> +	ret = axgbe_phy_sfp_get_mux(pdata);
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +		goto put;
> +	}
> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			&sfp_eeprom, sizeof(sfp_eeprom));
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +		goto put;
> +	}
> +
> +	data = info->data;
> +
> +	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
> +	if (info->length > AXGBE_SFP_EEPROM_PAGE_SIZE)
> +		for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +			data[i] = sfp_eeprom.base[i];
> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			&sfp_eeprom, sizeof(sfp_eeprom));
> +
> +	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
> +	if (info->length > AXGBE_SFP_EEPROM_PAGE_SIZE)
> +		for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +			data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] =
> +							sfp_eeprom.base[i];

The second page can be written partially, isn't this a problem?

> +
> +put:
> +	axgbe_phy_sfp_put_mux(pdata);
> +	axgbe_phy_put_comm_ownership(pdata);
> +	return ret;
> +}
> +
> +
>   static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
>   {
>   	struct axgbe_phy_data *phy_data = pdata->phy_data;
> 

Re: [dpdk-dev] [PATCH v1] net/axgbe: support sfp module EEPROM

[Ferruh Yigit]

On 4/8/2020 8:08 AM, asomalap@amd.com wrote:
> From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> 
> Adding API for get_module_eeprom and get_module_info.
> 
> Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
> ---
>  drivers/net/axgbe/axgbe_ethdev.c   |   2 +
>  drivers/net/axgbe/axgbe_phy.h      |   4 ++
>  drivers/net/axgbe/axgbe_phy_impl.c | 107 +++++++++++++++++++++++++++++
>  3 files changed, 113 insertions(+)
> 
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index 867058845..ea2f9bba1 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -214,6 +214,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
>  	.dev_supported_ptypes_get     = axgbe_dev_supported_ptypes_get,
>  	.rx_descriptor_status         = axgbe_dev_rx_descriptor_status,
>  	.tx_descriptor_status         = axgbe_dev_tx_descriptor_status,
> +	.get_module_info              = axgbe_get_module_info,
> +	.get_module_eeprom            = axgbe_get_module_eeprom,

Can you please update the 'axgbe.ini', to document the "Module EEPROM dump" feature?

<...>

> @@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
>  
>  #define AXGBE_SFP_EXTD_CC			31
>  
> +#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
> +
>  struct axgbe_sfp_eeprom {
>  	u8 base[64];
>  	u8 extd[32];
>  	u8 vendor[32];
>  };
>  
> +struct axgbe_sfp_eeprom_module {
> +	u8 base[256];

Is there any relation between this '256' and 'AXGBE_SFP_EEPROM_PAGE_SIZE' ?

<...>

> @@ -734,6 +740,106 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
>  	return ret;
>  }
>  
> +int axgbe_get_module_info(struct rte_eth_dev *dev,
> +				struct rte_eth_dev_module_info *modinfo)
> +{
> +	struct axgbe_port *pdata = dev->data->dev_private;
> +	struct axgbe_sfp_eeprom sfp_eeprom;
> +	uint8_t eeprom_addr;
> +	int ret;
> +
> +	ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +	if (ret)
> +		return -EIO;
> +
> +	ret = axgbe_phy_sfp_get_mux(pdata);
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +		return ret;

Should here put the ownsership back, 'axgbe_phy_put_comm_ownership()', ?

> +	}
> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			 &sfp_eeprom, sizeof(sfp_eeprom));
> +
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +		goto put;

In this case still returning success, shouldn't it return fail?

> +	}
> +
> +	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
> +		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
> +			modinfo->type = RTE_ETH_MODULE_SFF_8079;
> +			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
> +		} else {
> +			modinfo->type = RTE_ETH_MODULE_SFF_8472;
> +			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
> +		}
> +	}
> +
> +

Can you please remove extra empty line?

> +put:
> +	axgbe_phy_sfp_put_mux(pdata);
> +	axgbe_phy_put_comm_ownership(pdata);
> +	return 0;
> +}
> +
> +int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
> +				struct rte_dev_eeprom_info *info)
> +{
> +	struct axgbe_port *pdata = dev->data->dev_private;
> +	struct axgbe_sfp_eeprom_module sfp_eeprom;
> +	uint8_t eeprom_addr;
> +	uint8_t *data;
> +	uint32_t i;
> +	int ret;
> +
> +	ret = axgbe_phy_get_comm_ownership(pdata);
> +
> +	if (ret)
> +		return -EIO;
> +
> +	if (!info || !info->length || !info->data)
> +		return -EINVAL;

What do you think doing input validation before getting ownership?

> +
> +	ret = axgbe_phy_sfp_get_mux(pdata);
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
> +		return ret;
> +	}
> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			&sfp_eeprom, sizeof(sfp_eeprom));
> +	if (ret) {
> +		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
> +		goto put;
> +	}
> +	data = info->data;
> +
> +	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
> +	for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +		data[i] = sfp_eeprom.base[i];
> +
> +	eeprom_addr = 0;
> +	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
> +			&eeprom_addr, sizeof(eeprom_addr),
> +			&sfp_eeprom, sizeof(sfp_eeprom));
> +
> +	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
> +	for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
> +		data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = sfp_eeprom.base[i];

the provided offset information (info->offset) is not used at all, is this a
limitation or forgotten?

> +
> +put:
> +	axgbe_phy_sfp_put_mux(pdata);
> +	axgbe_phy_put_comm_ownership(pdata);
> +	return 0;
> +}
> +
>  static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
>  {
>  	struct axgbe_phy_data *phy_data = pdata->phy_data;
> @@ -741,6 +847,7 @@ static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
>  	u8 gpio_reg, gpio_ports[2];
>  	int ret;
>  
> +

This looks unrelated, can you please drop from the patch.

>  	/* Read the input port registers */
>  	gpio_reg = 0;
>  	ret = axgbe_phy_i2c_read(pdata, phy_data->sfp_gpio_address,
> 

Re: [dpdk-dev] [PATCH v1] net/axgbe: support sfp module EEPROM

[Kumar, Ravi1]

[AMD Public Use]

Acked-by: Ravi Kumar <ravi1.kumar@amd.com>
>
>
>-----Original Message-----
>From: Somalapuram, Amaranath <Amaranath.Somalapuram@amd.com> 
>Sent: Wednesday, April 8, 2020 12:38 PM
>To: dev@dpdk.org
>Cc: Kumar, Ravi1 <Ravi1.Kumar@amd.com>
>Subject: [PATCH v1] net/axgbe: support sfp module EEPROM
>
>From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
>
>Adding API for get_module_eeprom and get_module_info.
>
>Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
>---
> drivers/net/axgbe/axgbe_ethdev.c   |   2 +
> drivers/net/axgbe/axgbe_phy.h      |   4 ++
> drivers/net/axgbe/axgbe_phy_impl.c | 107 +++++++++++++++++++++++++++++
> 3 files changed, 113 insertions(+)
>
>diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
>index 867058845..ea2f9bba1 100644
>--- a/drivers/net/axgbe/axgbe_ethdev.c
>+++ b/drivers/net/axgbe/axgbe_ethdev.c
>@@ -214,6 +214,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
> 	.dev_supported_ptypes_get     = axgbe_dev_supported_ptypes_get,
> 	.rx_descriptor_status         = axgbe_dev_rx_descriptor_status,
> 	.tx_descriptor_status         = axgbe_dev_tx_descriptor_status,
>+	.get_module_info              = axgbe_get_module_info,
>+	.get_module_eeprom            = axgbe_get_module_eeprom,
> };
> 
> static int axgbe_phy_reset(struct axgbe_port *pdata) diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h index 77ee20a31..d9d7fde41 100644
>--- a/drivers/net/axgbe/axgbe_phy.h
>+++ b/drivers/net/axgbe/axgbe_phy.h
>@@ -188,5 +188,9 @@
> #define DUPLEX_FULL             0x01
> #define DUPLEX_UNKNOWN          0xff
> 
>+int axgbe_get_module_info(struct rte_eth_dev *dev,
>+				struct rte_eth_dev_module_info *modinfo); int 
>+axgbe_get_module_eeprom(struct rte_eth_dev *dev,
>+				  struct rte_dev_eeprom_info *info);
> #endif
> /* PHY */
>diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
>index 02236ec19..53716071f 100644
>--- a/drivers/net/axgbe/axgbe_phy_impl.c
>+++ b/drivers/net/axgbe/axgbe_phy_impl.c
>@@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
> 
> #define AXGBE_SFP_EXTD_CC			31
> 
>+#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
>+
> struct axgbe_sfp_eeprom {
> 	u8 base[64];
> 	u8 extd[32];
> 	u8 vendor[32];
> };
> 
>+struct axgbe_sfp_eeprom_module {
>+	u8 base[256];
>+};
>+
> #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
> #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
> 
>@@ -734,6 +740,106 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
> 	return ret;
> }
> 
>+int axgbe_get_module_info(struct rte_eth_dev *dev,
>+				struct rte_eth_dev_module_info *modinfo) {
>+	struct axgbe_port *pdata = dev->data->dev_private;
>+	struct axgbe_sfp_eeprom sfp_eeprom;
>+	uint8_t eeprom_addr;
>+	int ret;
>+
>+	ret = axgbe_phy_get_comm_ownership(pdata);
>+
>+	if (ret)
>+		return -EIO;
>+
>+	ret = axgbe_phy_sfp_get_mux(pdata);
>+
>+	if (ret) {
>+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
>+		return ret;
>+	}
>+
>+	eeprom_addr = 0;
>+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
>+			&eeprom_addr, sizeof(eeprom_addr),
>+			 &sfp_eeprom, sizeof(sfp_eeprom));
>+
>+	if (ret) {
>+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
>+		goto put;
>+	}
>+
>+	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
>+		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
>+			modinfo->type = RTE_ETH_MODULE_SFF_8079;
>+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
>+		} else {
>+			modinfo->type = RTE_ETH_MODULE_SFF_8472;
>+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
>+		}
>+	}
>+
>+
>+put:
>+	axgbe_phy_sfp_put_mux(pdata);
>+	axgbe_phy_put_comm_ownership(pdata);
>+	return 0;
>+}
>+
>+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
>+				struct rte_dev_eeprom_info *info)
>+{
>+	struct axgbe_port *pdata = dev->data->dev_private;
>+	struct axgbe_sfp_eeprom_module sfp_eeprom;
>+	uint8_t eeprom_addr;
>+	uint8_t *data;
>+	uint32_t i;
>+	int ret;
>+
>+	ret = axgbe_phy_get_comm_ownership(pdata);
>+
>+	if (ret)
>+		return -EIO;
>+
>+	if (!info || !info->length || !info->data)
>+		return -EINVAL;
>+
>+	ret = axgbe_phy_sfp_get_mux(pdata);
>+	if (ret) {
>+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
>+		return ret;
>+	}
>+
>+	eeprom_addr = 0;
>+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
>+			&eeprom_addr, sizeof(eeprom_addr),
>+			&sfp_eeprom, sizeof(sfp_eeprom));
>+	if (ret) {
>+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
>+		goto put;
>+	}
>+	data = info->data;
>+
>+	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
>+	for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
>+		data[i] = sfp_eeprom.base[i];
>+
>+	eeprom_addr = 0;
>+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
>+			&eeprom_addr, sizeof(eeprom_addr),
>+			&sfp_eeprom, sizeof(sfp_eeprom));
>+
>+	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
>+	for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
>+		data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = sfp_eeprom.base[i];
>+
>+put:
>+	axgbe_phy_sfp_put_mux(pdata);
>+	axgbe_phy_put_comm_ownership(pdata);
>+	return 0;
>+}
>+
> static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)  {
> 	struct axgbe_phy_data *phy_data = pdata->phy_data; @@ -741,6 +847,7 @@ static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
> 	u8 gpio_reg, gpio_ports[2];
> 	int ret;
> 
>+
> 	/* Read the input port registers */
> 	gpio_reg = 0;
> 	ret = axgbe_phy_i2c_read(pdata, phy_data->sfp_gpio_address,
>--
>2.17.1
>

Re: [dpdk-dev] [PATCH v1] net/axgbe: support sfp module EEPROM

[Somalapuram, Amaranath]

Hi Ravi,
Can you acknowledge this patch.

Regards,
S.Amarnath

-----Original Message-----
From: Somalapuram, Amaranath <asomalap@amd.com> 
Sent: Wednesday, April 8, 2020 12:38 PM
To: dev@dpdk.org
Cc: Kumar, Ravi1 <Ravi1.Kumar@amd.com>
Subject: [PATCH v1] net/axgbe: support sfp module EEPROM

From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>

Adding API for get_module_eeprom and get_module_info.

Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
---
 drivers/net/axgbe/axgbe_ethdev.c   |   2 +
 drivers/net/axgbe/axgbe_phy.h      |   4 ++
 drivers/net/axgbe/axgbe_phy_impl.c | 107 +++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)

diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 867058845..ea2f9bba1 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -214,6 +214,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
 	.dev_supported_ptypes_get     = axgbe_dev_supported_ptypes_get,
 	.rx_descriptor_status         = axgbe_dev_rx_descriptor_status,
 	.tx_descriptor_status         = axgbe_dev_tx_descriptor_status,
+	.get_module_info              = axgbe_get_module_info,
+	.get_module_eeprom            = axgbe_get_module_eeprom,
 };
 
 static int axgbe_phy_reset(struct axgbe_port *pdata) diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h index 77ee20a31..d9d7fde41 100644
--- a/drivers/net/axgbe/axgbe_phy.h
+++ b/drivers/net/axgbe/axgbe_phy.h
@@ -188,5 +188,9 @@
 #define DUPLEX_FULL             0x01
 #define DUPLEX_UNKNOWN          0xff
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo); int 
+axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				  struct rte_dev_eeprom_info *info);
 #endif
 /* PHY */
diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
index 02236ec19..53716071f 100644
--- a/drivers/net/axgbe/axgbe_phy_impl.c
+++ b/drivers/net/axgbe/axgbe_phy_impl.c
@@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
 
 #define AXGBE_SFP_EXTD_CC			31
 
+#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
+
 struct axgbe_sfp_eeprom {
 	u8 base[64];
 	u8 extd[32];
 	u8 vendor[32];
 };
 
+struct axgbe_sfp_eeprom_module {
+	u8 base[256];
+};
+
 #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
 #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
 
@@ -734,6 +740,106 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
 	return ret;
 }
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo) {
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom sfp_eeprom;
+	uint8_t eeprom_addr;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		return ret;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			 &sfp_eeprom, sizeof(sfp_eeprom));
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+
+	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
+		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
+			modinfo->type = RTE_ETH_MODULE_SFF_8079;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
+		} else {
+			modinfo->type = RTE_ETH_MODULE_SFF_8472;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
+		}
+	}
+
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return 0;
+}
+
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				struct rte_dev_eeprom_info *info)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom_module sfp_eeprom;
+	uint8_t eeprom_addr;
+	uint8_t *data;
+	uint32_t i;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	if (!info || !info->length || !info->data)
+		return -EINVAL;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		return ret;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+	data = info->data;
+
+	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
+	for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i] = sfp_eeprom.base[i];
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+
+	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
+	for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = sfp_eeprom.base[i];
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return 0;
+}
+
 static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)  {
 	struct axgbe_phy_data *phy_data = pdata->phy_data; @@ -741,6 +847,7 @@ static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
 	u8 gpio_reg, gpio_ports[2];
 	int ret;
 
+
 	/* Read the input port registers */
 	gpio_reg = 0;
 	ret = axgbe_phy_i2c_read(pdata, phy_data->sfp_gpio_address,
--
2.17.1

[dpdk-dev] [PATCH v1] net/axgbe: support sfp module EEPROM

[asomalap]

From: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>

Adding API for get_module_eeprom and get_module_info.

Signed-off-by: Amaranath Somalapuram <Amaranath.Somalapuram@amd.com>
---
 drivers/net/axgbe/axgbe_ethdev.c   |   2 +
 drivers/net/axgbe/axgbe_phy.h      |   4 ++
 drivers/net/axgbe/axgbe_phy_impl.c | 107 +++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+)

diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index 867058845..ea2f9bba1 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -214,6 +214,8 @@ static const struct eth_dev_ops axgbe_eth_dev_ops = {
 	.dev_supported_ptypes_get     = axgbe_dev_supported_ptypes_get,
 	.rx_descriptor_status         = axgbe_dev_rx_descriptor_status,
 	.tx_descriptor_status         = axgbe_dev_tx_descriptor_status,
+	.get_module_info              = axgbe_get_module_info,
+	.get_module_eeprom            = axgbe_get_module_eeprom,
 };
 
 static int axgbe_phy_reset(struct axgbe_port *pdata)
diff --git a/drivers/net/axgbe/axgbe_phy.h b/drivers/net/axgbe/axgbe_phy.h
index 77ee20a31..d9d7fde41 100644
--- a/drivers/net/axgbe/axgbe_phy.h
+++ b/drivers/net/axgbe/axgbe_phy.h
@@ -188,5 +188,9 @@
 #define DUPLEX_FULL             0x01
 #define DUPLEX_UNKNOWN          0xff
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo);
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				  struct rte_dev_eeprom_info *info);
 #endif
 /* PHY */
diff --git a/drivers/net/axgbe/axgbe_phy_impl.c b/drivers/net/axgbe/axgbe_phy_impl.c
index 02236ec19..53716071f 100644
--- a/drivers/net/axgbe/axgbe_phy_impl.c
+++ b/drivers/net/axgbe/axgbe_phy_impl.c
@@ -141,12 +141,18 @@ enum axgbe_sfp_speed {
 
 #define AXGBE_SFP_EXTD_CC			31
 
+#define AXGBE_SFP_EEPROM_PAGE_SIZE		256
+
 struct axgbe_sfp_eeprom {
 	u8 base[64];
 	u8 extd[32];
 	u8 vendor[32];
 };
 
+struct axgbe_sfp_eeprom_module {
+	u8 base[256];
+};
+
 #define AXGBE_BEL_FUSE_VENDOR	"BEL-FUSE"
 #define AXGBE_BEL_FUSE_PARTNO	"1GBT-SFP06"
 
@@ -734,6 +740,106 @@ static int axgbe_phy_sfp_read_eeprom(struct axgbe_port *pdata)
 	return ret;
 }
 
+int axgbe_get_module_info(struct rte_eth_dev *dev,
+				struct rte_eth_dev_module_info *modinfo)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom sfp_eeprom;
+	uint8_t eeprom_addr;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		return ret;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			 &sfp_eeprom, sizeof(sfp_eeprom));
+
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+
+	if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] != 0xff) {
+		if (sfp_eeprom.extd[AXGBE_SFP_EXTD_SFF_8472] == 0) {
+			modinfo->type = RTE_ETH_MODULE_SFF_8079;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8079_LEN;
+		} else {
+			modinfo->type = RTE_ETH_MODULE_SFF_8472;
+			modinfo->eeprom_len = RTE_ETH_MODULE_SFF_8472_LEN;
+		}
+	}
+
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return 0;
+}
+
+int axgbe_get_module_eeprom(struct rte_eth_dev *dev,
+				struct rte_dev_eeprom_info *info)
+{
+	struct axgbe_port *pdata = dev->data->dev_private;
+	struct axgbe_sfp_eeprom_module sfp_eeprom;
+	uint8_t eeprom_addr;
+	uint8_t *data;
+	uint32_t i;
+	int ret;
+
+	ret = axgbe_phy_get_comm_ownership(pdata);
+
+	if (ret)
+		return -EIO;
+
+	if (!info || !info->length || !info->data)
+		return -EINVAL;
+
+	ret = axgbe_phy_sfp_get_mux(pdata);
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error setting SFP MUX\n");
+		return ret;
+	}
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_SERIAL_ID_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+	if (ret) {
+		PMD_DRV_LOG(ERR, "I2C error reading SFP EEPROM\n");
+		goto put;
+	}
+	data = info->data;
+
+	/* for AXGBE_SFP_SERIAL_ID_ADDRESS */
+	for (i = 0; i < AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i] = sfp_eeprom.base[i];
+
+	eeprom_addr = 0;
+	ret = axgbe_phy_i2c_read(pdata, AXGBE_SFP_DIAG_INFO_ADDRESS,
+			&eeprom_addr, sizeof(eeprom_addr),
+			&sfp_eeprom, sizeof(sfp_eeprom));
+
+	/* for AXGBE_SFP_DIAG_INFO_ADDRESS */
+	for (i = 0; i < info->length - AXGBE_SFP_EEPROM_PAGE_SIZE; i++)
+		data[i + AXGBE_SFP_EEPROM_PAGE_SIZE] = sfp_eeprom.base[i];
+
+put:
+	axgbe_phy_sfp_put_mux(pdata);
+	axgbe_phy_put_comm_ownership(pdata);
+	return 0;
+}
+
 static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
 {
 	struct axgbe_phy_data *phy_data = pdata->phy_data;
@@ -741,6 +847,7 @@ static void axgbe_phy_sfp_signals(struct axgbe_port *pdata)
 	u8 gpio_reg, gpio_ports[2];
 	int ret;
 
+
 	/* Read the input port registers */
 	gpio_reg = 0;
 	ret = axgbe_phy_i2c_read(pdata, phy_data->sfp_gpio_address,
-- 
2.17.1