[PATCH] net/nfp: write link speed to control BAR

[PATCH] net/nfp: write link speed to control BAR

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

Due to changes in the firmware for NFPs, firmware will no longer write
the link speed of a port to the control BAR. In line with the behaviour
of the kernel NFP driver, this is now handled by the PMD by reading the
value provided by the NSP in the nfp_eth_table struct within the pf_dev
of the port and subsequently writing this value to the control BAR.

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/nfp_common.c | 90 ++++++++++++++++++++++--------------
 drivers/net/nfp/nfp_ctrl.h   |  9 ++++
 2 files changed, 65 insertions(+), 34 deletions(-)

diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
index 5922bfea8e..006ea58008 100644
--- a/drivers/net/nfp/nfp_common.c
+++ b/drivers/net/nfp/nfp_common.c
@@ -52,6 +52,53 @@
 #include <sys/ioctl.h>
 #include <errno.h>
 
+static const uint32_t nfp_net_link_speed_nfp2rte[] = {
+	[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = RTE_ETH_SPEED_NUM_NONE,
+	[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = RTE_ETH_SPEED_NUM_NONE,
+	[NFP_NET_CFG_STS_LINK_RATE_1G]          = RTE_ETH_SPEED_NUM_1G,
+	[NFP_NET_CFG_STS_LINK_RATE_10G]         = RTE_ETH_SPEED_NUM_10G,
+	[NFP_NET_CFG_STS_LINK_RATE_25G]         = RTE_ETH_SPEED_NUM_25G,
+	[NFP_NET_CFG_STS_LINK_RATE_40G]         = RTE_ETH_SPEED_NUM_40G,
+	[NFP_NET_CFG_STS_LINK_RATE_50G]         = RTE_ETH_SPEED_NUM_50G,
+	[NFP_NET_CFG_STS_LINK_RATE_100G]        = RTE_ETH_SPEED_NUM_100G,
+};
+
+static uint32_t
+nfp_net_link_speed_rte2nfp(uint32_t speed)
+{
+	uint32_t i;
+
+	for (i = 0; i < RTE_DIM(nfp_net_link_speed_nfp2rte); i++) {
+		if (speed == nfp_net_link_speed_nfp2rte[i])
+			return i;
+	}
+
+	return NFP_NET_CFG_STS_LINK_RATE_UNKNOWN;
+}
+
+static void
+nfp_net_notify_port_speed(struct rte_eth_dev *dev)
+{
+	struct nfp_net_hw *hw;
+	struct nfp_eth_table *eth_table;
+	uint32_t nn_link_status;
+
+	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	eth_table = hw->pf_dev->nfp_eth_table;
+
+	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
+	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
+			NFP_NET_CFG_STS_LINK_RATE_MASK;
+
+	if ((nn_link_status & NFP_NET_CFG_STS_LINK) == 0) {
+		nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE, NFP_NET_CFG_STS_LINK_RATE_UNKNOWN);
+		return;
+	}
+
+	nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
+		      nfp_net_link_speed_rte2nfp(eth_table->ports[hw->idx].speed));
+}
+
 static int
 __nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t update)
 {
@@ -111,6 +158,9 @@ nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t ctrl, uint32_t update)
 	PMD_DRV_LOG(DEBUG, "nfp_net_reconfig: ctrl=%08x update=%08x",
 		    ctrl, update);
 
+	if (hw->pf_dev != NULL && hw->pf_dev->app_fw_id == NFP_APP_FW_CORE_NIC)
+		nfp_net_notify_port_speed(hw->eth_dev);
+
 	rte_spinlock_lock(&hw->reconfig_lock);
 
 	nn_cfg_writel(hw, NFP_NET_CFG_CTRL, ctrl);
@@ -538,22 +588,9 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 {
 	struct nfp_net_hw *hw;
 	struct rte_eth_link link;
-	struct nfp_eth_table *nfp_eth_table;
 	uint32_t nn_link_status;
-	uint32_t i;
 	int ret;
 
-	static const uint32_t ls_to_ethtool[] = {
-		[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = RTE_ETH_SPEED_NUM_NONE,
-		[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = RTE_ETH_SPEED_NUM_NONE,
-		[NFP_NET_CFG_STS_LINK_RATE_1G]          = RTE_ETH_SPEED_NUM_1G,
-		[NFP_NET_CFG_STS_LINK_RATE_10G]         = RTE_ETH_SPEED_NUM_10G,
-		[NFP_NET_CFG_STS_LINK_RATE_25G]         = RTE_ETH_SPEED_NUM_25G,
-		[NFP_NET_CFG_STS_LINK_RATE_40G]         = RTE_ETH_SPEED_NUM_40G,
-		[NFP_NET_CFG_STS_LINK_RATE_50G]         = RTE_ETH_SPEED_NUM_50G,
-		[NFP_NET_CFG_STS_LINK_RATE_100G]        = RTE_ETH_SPEED_NUM_100G,
-	};
-
 	PMD_DRV_LOG(DEBUG, "Link update");
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -567,28 +604,13 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 
 	link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
 
-	if (hw->pf_dev != NULL) {
-		nfp_eth_table = hw->pf_dev->nfp_eth_table;
-		if (nfp_eth_table != NULL) {
-			link.link_speed = nfp_eth_table->ports[hw->idx].speed;
-			for (i = 0; i < RTE_DIM(ls_to_ethtool); i++) {
-				if (ls_to_ethtool[i] == link.link_speed)
-					break;
-			}
-			if (i == RTE_DIM(ls_to_ethtool))
-				link.link_speed = RTE_ETH_SPEED_NUM_NONE;
-		} else {
-			link.link_speed = RTE_ETH_SPEED_NUM_NONE;
-		}
-	} else {
-		nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
-				NFP_NET_CFG_STS_LINK_RATE_MASK;
+	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
+			 NFP_NET_CFG_STS_LINK_RATE_MASK;
 
-		if (nn_link_status >= RTE_DIM(ls_to_ethtool))
-			link.link_speed = RTE_ETH_SPEED_NUM_NONE;
-		else
-			link.link_speed = ls_to_ethtool[nn_link_status];
-	}
+	if (nn_link_status >= RTE_DIM(nfp_net_link_speed_nfp2rte))
+		link.link_speed = RTE_ETH_SPEED_NUM_NONE;
+	else
+		link.link_speed = nfp_net_link_speed_nfp2rte[nn_link_status];
 
 	ret = rte_eth_linkstatus_set(dev, &link);
 	if (ret == 0) {
diff --git a/drivers/net/nfp/nfp_ctrl.h b/drivers/net/nfp/nfp_ctrl.h
index 75e4458a1b..244048796c 100644
--- a/drivers/net/nfp/nfp_ctrl.h
+++ b/drivers/net/nfp/nfp_ctrl.h
@@ -178,6 +178,15 @@
 #define   NFP_NET_CFG_STS_LINK_RATE_40G           5
 #define   NFP_NET_CFG_STS_LINK_RATE_50G           6
 #define   NFP_NET_CFG_STS_LINK_RATE_100G          7
+
+/*
+ * NSP Link rate is a 16-bit word. It is no longer determined by
+ * firmware, instead it is read from the nfp_eth_table of the
+ * associated pf_dev and written to the NFP_NET_CFG_STS_NSP_LINK_RATE
+ * address by the PMD each time the port is reconfigured.
+ */
+#define NFP_NET_CFG_STS_NSP_LINK_RATE   0x0036
+
 #define NFP_NET_CFG_CAP                 0x0038
 #define NFP_NET_CFG_MAX_TXRINGS         0x003c
 #define NFP_NET_CFG_MAX_RXRINGS         0x0040
-- 
2.29.3

Re: [PATCH] net/nfp: write link speed to control BAR

[Ferruh Yigit]

On 2/21/2023 6:29 AM, Chaoyong He wrote:
> From: James Hershaw <james.hershaw@corigine.com>
> 
> Due to changes in the firmware for NFPs, firmware will no longer write
> the link speed of a port to the control BAR. In line with the behaviour
> of the kernel NFP driver, this is now handled by the PMD by reading the
> value provided by the NSP in the nfp_eth_table struct within the pf_dev
> of the port and subsequently writing this value to the control BAR.
> 

Don't you need some kind of FW version check to figure out if
'NFP_NET_CFG_STS_NSP_LINK_RATE' needs to be updated by driver or not?

How do you manage driver <-> FW dependency?


> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>  drivers/net/nfp/nfp_common.c | 90 ++++++++++++++++++++++--------------
>  drivers/net/nfp/nfp_ctrl.h   |  9 ++++
>  2 files changed, 65 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
> index 5922bfea8e..006ea58008 100644
> --- a/drivers/net/nfp/nfp_common.c
> +++ b/drivers/net/nfp/nfp_common.c
> @@ -52,6 +52,53 @@
>  #include <sys/ioctl.h>
>  #include <errno.h>
>  
> +static const uint32_t nfp_net_link_speed_nfp2rte[] = {
> +	[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = RTE_ETH_SPEED_NUM_NONE,
> +	[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = RTE_ETH_SPEED_NUM_NONE,
> +	[NFP_NET_CFG_STS_LINK_RATE_1G]          = RTE_ETH_SPEED_NUM_1G,
> +	[NFP_NET_CFG_STS_LINK_RATE_10G]         = RTE_ETH_SPEED_NUM_10G,
> +	[NFP_NET_CFG_STS_LINK_RATE_25G]         = RTE_ETH_SPEED_NUM_25G,
> +	[NFP_NET_CFG_STS_LINK_RATE_40G]         = RTE_ETH_SPEED_NUM_40G,
> +	[NFP_NET_CFG_STS_LINK_RATE_50G]         = RTE_ETH_SPEED_NUM_50G,
> +	[NFP_NET_CFG_STS_LINK_RATE_100G]        = RTE_ETH_SPEED_NUM_100G,
> +};
> +
> +static uint32_t
> +nfp_net_link_speed_rte2nfp(uint32_t speed)
> +{
> +	uint32_t i;
> +
> +	for (i = 0; i < RTE_DIM(nfp_net_link_speed_nfp2rte); i++) {
> +		if (speed == nfp_net_link_speed_nfp2rte[i])
> +			return i;
> +	}
> +
> +	return NFP_NET_CFG_STS_LINK_RATE_UNKNOWN;
> +}
> +
> +static void
> +nfp_net_notify_port_speed(struct rte_eth_dev *dev)
> +{
> +	struct nfp_net_hw *hw;
> +	struct nfp_eth_table *eth_table;
> +	uint32_t nn_link_status;
> +
> +	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	eth_table = hw->pf_dev->nfp_eth_table;
> +
> +	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
> +	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> +			NFP_NET_CFG_STS_LINK_RATE_MASK;
> +
> +	if ((nn_link_status & NFP_NET_CFG_STS_LINK) == 0) {
> +		nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE, NFP_NET_CFG_STS_LINK_RATE_UNKNOWN);
> +		return;
> +	}
> +
> +	nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
> +		      nfp_net_link_speed_rte2nfp(eth_table->ports[hw->idx].speed));

PF driver writes link speed to 'NFP_NET_CFG_STS_NSP_LINK_RATE' register,
but 'nfp_net_link_update()' still gets it from 'NFP_NET_CFG_STS'
register (via 'nfp_net_link_speed_nfp2rte[nn_link_status]').

Shouldn't 'nfp_net_link_update()' needs to be updated to read speed from
'NFP_NET_CFG_STS_NSP_LINK_RATE' register?

RE: [PATCH] net/nfp: write link speed to control BAR

[Chaoyong He]

> On 2/21/2023 6:29 AM, Chaoyong He wrote:
> > From: James Hershaw <james.hershaw@corigine.com>
> >
> > Due to changes in the firmware for NFPs, firmware will no longer write
> > the link speed of a port to the control BAR. In line with the
> > behaviour of the kernel NFP driver, this is now handled by the PMD by
> > reading the value provided by the NSP in the nfp_eth_table struct
> > within the pf_dev of the port and subsequently writing this value to the
> control BAR.
> >
> 
> Don't you need some kind of FW version check to figure out if
> 'NFP_NET_CFG_STS_NSP_LINK_RATE' needs to be updated by driver or not?
> 
> How do you manage driver <-> FW dependency?
> 
> 
> > Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > ---
> >  drivers/net/nfp/nfp_common.c | 90 ++++++++++++++++++++++-----------
> ---
> >  drivers/net/nfp/nfp_ctrl.h   |  9 ++++
> >  2 files changed, 65 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/net/nfp/nfp_common.c
> > b/drivers/net/nfp/nfp_common.c index 5922bfea8e..006ea58008 100644
> > --- a/drivers/net/nfp/nfp_common.c
> > +++ b/drivers/net/nfp/nfp_common.c
> > @@ -52,6 +52,53 @@
> >  #include <sys/ioctl.h>
> >  #include <errno.h>
> >
> > +static const uint32_t nfp_net_link_speed_nfp2rte[] = {
> > +	[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] =
> RTE_ETH_SPEED_NUM_NONE,
> > +	[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     =
> RTE_ETH_SPEED_NUM_NONE,
> > +	[NFP_NET_CFG_STS_LINK_RATE_1G]          =
> RTE_ETH_SPEED_NUM_1G,
> > +	[NFP_NET_CFG_STS_LINK_RATE_10G]         =
> RTE_ETH_SPEED_NUM_10G,
> > +	[NFP_NET_CFG_STS_LINK_RATE_25G]         =
> RTE_ETH_SPEED_NUM_25G,
> > +	[NFP_NET_CFG_STS_LINK_RATE_40G]         =
> RTE_ETH_SPEED_NUM_40G,
> > +	[NFP_NET_CFG_STS_LINK_RATE_50G]         =
> RTE_ETH_SPEED_NUM_50G,
> > +	[NFP_NET_CFG_STS_LINK_RATE_100G]        =
> RTE_ETH_SPEED_NUM_100G,
> > +};
> > +
> > +static uint32_t
> > +nfp_net_link_speed_rte2nfp(uint32_t speed) {
> > +	uint32_t i;
> > +
> > +	for (i = 0; i < RTE_DIM(nfp_net_link_speed_nfp2rte); i++) {
> > +		if (speed == nfp_net_link_speed_nfp2rte[i])
> > +			return i;
> > +	}
> > +
> > +	return NFP_NET_CFG_STS_LINK_RATE_UNKNOWN;
> > +}
> > +
> > +static void
> > +nfp_net_notify_port_speed(struct rte_eth_dev *dev) {
> > +	struct nfp_net_hw *hw;
> > +	struct nfp_eth_table *eth_table;
> > +	uint32_t nn_link_status;
> > +
> > +	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +	eth_table = hw->pf_dev->nfp_eth_table;
> > +
> > +	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
> > +	nn_link_status = (nn_link_status >>
> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> > +			NFP_NET_CFG_STS_LINK_RATE_MASK;
> > +
> > +	if ((nn_link_status & NFP_NET_CFG_STS_LINK) == 0) {
> > +		nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
> NFP_NET_CFG_STS_LINK_RATE_UNKNOWN);
> > +		return;
> > +	}
> > +
> > +	nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
> > +		      nfp_net_link_speed_rte2nfp(eth_table->ports[hw-
> >idx].speed));
> 
> PF driver writes link speed to 'NFP_NET_CFG_STS_NSP_LINK_RATE' register,
> but 'nfp_net_link_update()' still gets it from 'NFP_NET_CFG_STS'
> register (via 'nfp_net_link_speed_nfp2rte[nn_link_status]').
> 
> Shouldn't 'nfp_net_link_update()' needs to be updated to read speed from
> 'NFP_NET_CFG_STS_NSP_LINK_RATE' register?

Sorry for the late response, we spend a lot of time to check and discuss.

For older firmware, a full word is allocated (NFP_NET_CFG_STS) to report link status and port speed to the driver.
However, in the interests of keeping FW files port-speed agnostic in the future, 
the upper 16 bits are no longer written to by FW, so we write the speed to that address (NFP_NET_CFG_STS_LINK_RATE).
The lower 16 bits (link status) are still handled by firmware.

These changes are completely backwards compatible with older firmware versions, so no FW version check is required.

Re: [PATCH] net/nfp: write link speed to control BAR

[Ferruh Yigit]

On 3/6/2023 7:06 AM, Chaoyong He wrote:
>> On 2/21/2023 6:29 AM, Chaoyong He wrote:
>>> From: James Hershaw <james.hershaw@corigine.com>
>>>
>>> Due to changes in the firmware for NFPs, firmware will no longer write
>>> the link speed of a port to the control BAR. In line with the
>>> behaviour of the kernel NFP driver, this is now handled by the PMD by
>>> reading the value provided by the NSP in the nfp_eth_table struct
>>> within the pf_dev of the port and subsequently writing this value to the
>> control BAR.
>>>
>>
>> Don't you need some kind of FW version check to figure out if
>> 'NFP_NET_CFG_STS_NSP_LINK_RATE' needs to be updated by driver or not?
>>
>> How do you manage driver <-> FW dependency?
>>
>>
>>> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>> ---
>>>  drivers/net/nfp/nfp_common.c | 90 ++++++++++++++++++++++-----------
>> ---
>>>  drivers/net/nfp/nfp_ctrl.h   |  9 ++++
>>>  2 files changed, 65 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/net/nfp/nfp_common.c
>>> b/drivers/net/nfp/nfp_common.c index 5922bfea8e..006ea58008 100644
>>> --- a/drivers/net/nfp/nfp_common.c
>>> +++ b/drivers/net/nfp/nfp_common.c
>>> @@ -52,6 +52,53 @@
>>>  #include <sys/ioctl.h>
>>>  #include <errno.h>
>>>
>>> +static const uint32_t nfp_net_link_speed_nfp2rte[] = {
>>> +	[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] =
>> RTE_ETH_SPEED_NUM_NONE,
>>> +	[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     =
>> RTE_ETH_SPEED_NUM_NONE,
>>> +	[NFP_NET_CFG_STS_LINK_RATE_1G]          =
>> RTE_ETH_SPEED_NUM_1G,
>>> +	[NFP_NET_CFG_STS_LINK_RATE_10G]         =
>> RTE_ETH_SPEED_NUM_10G,
>>> +	[NFP_NET_CFG_STS_LINK_RATE_25G]         =
>> RTE_ETH_SPEED_NUM_25G,
>>> +	[NFP_NET_CFG_STS_LINK_RATE_40G]         =
>> RTE_ETH_SPEED_NUM_40G,
>>> +	[NFP_NET_CFG_STS_LINK_RATE_50G]         =
>> RTE_ETH_SPEED_NUM_50G,
>>> +	[NFP_NET_CFG_STS_LINK_RATE_100G]        =
>> RTE_ETH_SPEED_NUM_100G,
>>> +};
>>> +
>>> +static uint32_t
>>> +nfp_net_link_speed_rte2nfp(uint32_t speed) {
>>> +	uint32_t i;
>>> +
>>> +	for (i = 0; i < RTE_DIM(nfp_net_link_speed_nfp2rte); i++) {
>>> +		if (speed == nfp_net_link_speed_nfp2rte[i])
>>> +			return i;
>>> +	}
>>> +
>>> +	return NFP_NET_CFG_STS_LINK_RATE_UNKNOWN;
>>> +}
>>> +
>>> +static void
>>> +nfp_net_notify_port_speed(struct rte_eth_dev *dev) {
>>> +	struct nfp_net_hw *hw;
>>> +	struct nfp_eth_table *eth_table;
>>> +	uint32_t nn_link_status;
>>> +
>>> +	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>> +	eth_table = hw->pf_dev->nfp_eth_table;
>>> +
>>> +	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
>>> +	nn_link_status = (nn_link_status >>
>> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
>>> +			NFP_NET_CFG_STS_LINK_RATE_MASK;
>>> +
>>> +	if ((nn_link_status & NFP_NET_CFG_STS_LINK) == 0) {
>>> +		nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
>> NFP_NET_CFG_STS_LINK_RATE_UNKNOWN);
>>> +		return;
>>> +	}
>>> +
>>> +	nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
>>> +		      nfp_net_link_speed_rte2nfp(eth_table->ports[hw-
>>> idx].speed));
>>
>> PF driver writes link speed to 'NFP_NET_CFG_STS_NSP_LINK_RATE' register,
>> but 'nfp_net_link_update()' still gets it from 'NFP_NET_CFG_STS'
>> register (via 'nfp_net_link_speed_nfp2rte[nn_link_status]').
>>
>> Shouldn't 'nfp_net_link_update()' needs to be updated to read speed from
>> 'NFP_NET_CFG_STS_NSP_LINK_RATE' register?
> 
> Sorry for the late response, we spend a lot of time to check and discuss.
> 
> For older firmware, a full word is allocated (NFP_NET_CFG_STS) to report link status and port speed to the driver.
> However, in the interests of keeping FW files port-speed agnostic in the future, 
> the upper 16 bits are no longer written to by FW, so we write the speed to that address (NFP_NET_CFG_STS_LINK_RATE).
> The lower 16 bits (link status) are still handled by firmware.
> 

But 'nfp_net_link_update()' still gets the links speed from lower 16
bits. Probably I am missing something but please let me understand.


link_update()              notify_port_speed()
 read(speed)                writel(speed)
  ▲                          │
  │                          │
  │                          │
 ┌┴─────────────────────────┐▼────────────────────────┐
 │                          │                         │
 │                          │    LINK_RATE            │
 └──────────────────────────┴─────────────────────────┘
0x34                       0x36
 │                                                    │
 └──────────────── CFG_STS ───────────────────────────┘


Or is it something like when you update upper half of the register, FW
reads it and reflects the value to the lower half of the register?


And since 'NFP_NET_CFG_STS_NSP_LINK_RATE' is 16 bits, is it correct to
use 'nn_cfg_writel()' to update it?

> These changes are completely backwards compatible with older firmware versions, so no FW version check is required.

ack

RE: [PATCH] net/nfp: write link speed to control BAR

[Chaoyong He]

> On 3/6/2023 7:06 AM, Chaoyong He wrote:
> >> On 2/21/2023 6:29 AM, Chaoyong He wrote:
> >>> From: James Hershaw <james.hershaw@corigine.com>
> >>>
> >>> Due to changes in the firmware for NFPs, firmware will no longer
> >>> write the link speed of a port to the control BAR. In line with the
> >>> behaviour of the kernel NFP driver, this is now handled by the PMD
> >>> by reading the value provided by the NSP in the nfp_eth_table struct
> >>> within the pf_dev of the port and subsequently writing this value to
> >>> the
> >> control BAR.
> >>>
> >>
> >> Don't you need some kind of FW version check to figure out if
> >> 'NFP_NET_CFG_STS_NSP_LINK_RATE' needs to be updated by driver or
> not?
> >>
> >> How do you manage driver <-> FW dependency?
> >>
> >>
> >>> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> >>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> >>> ---
> >>>  drivers/net/nfp/nfp_common.c | 90 ++++++++++++++++++++++-------
> ----
> >> ---
> >>>  drivers/net/nfp/nfp_ctrl.h   |  9 ++++
> >>>  2 files changed, 65 insertions(+), 34 deletions(-)
> >>>
> >>> diff --git a/drivers/net/nfp/nfp_common.c
> >>> b/drivers/net/nfp/nfp_common.c index 5922bfea8e..006ea58008
> 100644
> >>> --- a/drivers/net/nfp/nfp_common.c
> >>> +++ b/drivers/net/nfp/nfp_common.c
> >>> @@ -52,6 +52,53 @@
> >>>  #include <sys/ioctl.h>
> >>>  #include <errno.h>
> >>>
> >>> +static const uint32_t nfp_net_link_speed_nfp2rte[] = {
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] =
> >> RTE_ETH_SPEED_NUM_NONE,
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     =
> >> RTE_ETH_SPEED_NUM_NONE,
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_1G]          =
> >> RTE_ETH_SPEED_NUM_1G,
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_10G]         =
> >> RTE_ETH_SPEED_NUM_10G,
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_25G]         =
> >> RTE_ETH_SPEED_NUM_25G,
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_40G]         =
> >> RTE_ETH_SPEED_NUM_40G,
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_50G]         =
> >> RTE_ETH_SPEED_NUM_50G,
> >>> +	[NFP_NET_CFG_STS_LINK_RATE_100G]        =
> >> RTE_ETH_SPEED_NUM_100G,
> >>> +};
> >>> +
> >>> +static uint32_t
> >>> +nfp_net_link_speed_rte2nfp(uint32_t speed) {
> >>> +	uint32_t i;
> >>> +
> >>> +	for (i = 0; i < RTE_DIM(nfp_net_link_speed_nfp2rte); i++) {
> >>> +		if (speed == nfp_net_link_speed_nfp2rte[i])
> >>> +			return i;
> >>> +	}
> >>> +
> >>> +	return NFP_NET_CFG_STS_LINK_RATE_UNKNOWN;
> >>> +}
> >>> +
> >>> +static void
> >>> +nfp_net_notify_port_speed(struct rte_eth_dev *dev) {
> >>> +	struct nfp_net_hw *hw;
> >>> +	struct nfp_eth_table *eth_table;
> >>> +	uint32_t nn_link_status;
> >>> +
> >>> +	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >>> +	eth_table = hw->pf_dev->nfp_eth_table;
> >>> +
> >>> +	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
> >>> +	nn_link_status = (nn_link_status >>
> >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> >>> +			NFP_NET_CFG_STS_LINK_RATE_MASK;
> >>> +
> >>> +	if ((nn_link_status & NFP_NET_CFG_STS_LINK) == 0) {
> >>> +		nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
> >> NFP_NET_CFG_STS_LINK_RATE_UNKNOWN);
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	nn_cfg_writel(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
> >>> +		      nfp_net_link_speed_rte2nfp(eth_table->ports[hw-
> >>> idx].speed));
> >>
> >> PF driver writes link speed to 'NFP_NET_CFG_STS_NSP_LINK_RATE'
> >> register, but 'nfp_net_link_update()' still gets it from 'NFP_NET_CFG_STS'
> >> register (via 'nfp_net_link_speed_nfp2rte[nn_link_status]').
> >>
> >> Shouldn't 'nfp_net_link_update()' needs to be updated to read speed
> >> from 'NFP_NET_CFG_STS_NSP_LINK_RATE' register?
> >
> > Sorry for the late response, we spend a lot of time to check and discuss.
> >
> > For older firmware, a full word is allocated (NFP_NET_CFG_STS) to report
> link status and port speed to the driver.
> > However, in the interests of keeping FW files port-speed agnostic in
> > the future, the upper 16 bits are no longer written to by FW, so we write
> the speed to that address (NFP_NET_CFG_STS_LINK_RATE).
> > The lower 16 bits (link status) are still handled by firmware.
> >
> 
> But 'nfp_net_link_update()' still gets the links speed from lower 16 bits.
> Probably I am missing something but please let me understand.
> 
> 
> link_update()              notify_port_speed()
>  read(speed)                writel(speed)
>   ▲                          │
>   │                          │
>   │                          │
>  ┌┴─────────────────────────┐▼──
> ──────────────────────┐
>  │                          │                         │
>  │                          │    LINK_RATE            │
>  └──────────────────────────┴───
> ──────────────────────┘
> 0x34                       0x36
>  │                                                    │
>  └──────────────── CFG_STS ──────────
> ─────────────────┘
> 
> 
> Or is it something like when you update upper half of the register, FW reads
> it and reflects the value to the lower half of the register?
> 
> 
> And since 'NFP_NET_CFG_STS_NSP_LINK_RATE' is 16 bits, is it correct to use
> 'nn_cfg_writel()' to update it?
> 

Oh, yes, thanks for your careful review, here we do make a mistake, sorry for that.
We'll send a v2 patch with the right 16bits read/write api soon, thanks again.

> > These changes are completely backwards compatible with older firmware
> versions, so no FW version check is required.
> 
> ack

[PATCH v2 0/2] write link speed to control BAR

[Chaoyong He]

Due to changes in the firmware for NFPs, firmware will no longer write
the link speed of a port to the control BAR. In line with the behaviour
of the kernel NFP driver, this is now handled by the PMD by reading the
value provided by the NSP in the nfp_eth_table struct within the pf_dev
of the port and subsequently writing this value to the control BAR.

---
V2:
* Using the 16bit read/write helper functions
---
James Hershaw (2):
  net/nfp: add helper functions for read/write 16b values
  net/nfp: write link speed to control BAR

 drivers/net/nfp/nfp_common.c | 104 +++++++++++++++++++++++------------
 drivers/net/nfp/nfp_common.h |  17 ++++++
 drivers/net/nfp/nfp_ctrl.h   |   9 +++
 3 files changed, 94 insertions(+), 36 deletions(-)

-- 
2.39.1

[PATCH v2 1/2] net/nfp: add helper functions for read/write 16b values

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

Expand the selection of read/write helper functions to include values of
16 bits.

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/nfp_common.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/nfp/nfp_common.h b/drivers/net/nfp/nfp_common.h
index 4486ffa72c..3a8b023c75 100644
--- a/drivers/net/nfp/nfp_common.h
+++ b/drivers/net/nfp/nfp_common.h
@@ -283,6 +283,11 @@ static inline void nn_writel(uint32_t val, volatile void *addr)
 	rte_write32(val, addr);
 }
 
+static inline uint16_t nn_readw(volatile const void *addr)
+{
+	return rte_read16(addr);
+}
+
 static inline void nn_writew(uint16_t val, volatile void *addr)
 {
 	rte_write16(val, addr);
@@ -321,6 +326,18 @@ nn_cfg_writeb(struct nfp_net_hw *hw, int off, uint8_t val)
 	nn_writeb(val, hw->ctrl_bar + off);
 }
 
+static inline uint16_t
+nn_cfg_readw(struct nfp_net_hw *hw, int off)
+{
+	return rte_le_to_cpu_16(nn_readw(hw->ctrl_bar + off));
+}
+
+static inline void
+nn_cfg_writew(struct nfp_net_hw *hw, int off, uint16_t val)
+{
+	nn_writew(rte_cpu_to_le_16(val), hw->ctrl_bar + off);
+}
+
 static inline uint32_t
 nn_cfg_readl(struct nfp_net_hw *hw, int off)
 {
-- 
2.39.1

[PATCH v2 2/2] net/nfp: write link speed to control BAR

[Chaoyong He]

From: James Hershaw <james.hershaw@corigine.com>

Due to changes in the firmware for NFPs, firmware will no longer write
the link speed of a port to the control BAR. In line with the behaviour
of the kernel NFP driver, this is now handled by the PMD by reading the
value provided by the NSP in the nfp_eth_table struct within the pf_dev
of the port and subsequently writing this value to the control BAR.

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/nfp_common.c | 104 +++++++++++++++++++++++------------
 drivers/net/nfp/nfp_ctrl.h   |   9 +++
 2 files changed, 77 insertions(+), 36 deletions(-)

diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
index 5d92b476e2..2261d955c8 100644
--- a/drivers/net/nfp/nfp_common.c
+++ b/drivers/net/nfp/nfp_common.c
@@ -52,6 +52,58 @@
 #include <sys/ioctl.h>
 #include <errno.h>
 
+static const uint32_t nfp_net_link_speed_nfp2rte[] = {
+	[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = RTE_ETH_SPEED_NUM_NONE,
+	[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = RTE_ETH_SPEED_NUM_NONE,
+	[NFP_NET_CFG_STS_LINK_RATE_1G]          = RTE_ETH_SPEED_NUM_1G,
+	[NFP_NET_CFG_STS_LINK_RATE_10G]         = RTE_ETH_SPEED_NUM_10G,
+	[NFP_NET_CFG_STS_LINK_RATE_25G]         = RTE_ETH_SPEED_NUM_25G,
+	[NFP_NET_CFG_STS_LINK_RATE_40G]         = RTE_ETH_SPEED_NUM_40G,
+	[NFP_NET_CFG_STS_LINK_RATE_50G]         = RTE_ETH_SPEED_NUM_50G,
+	[NFP_NET_CFG_STS_LINK_RATE_100G]        = RTE_ETH_SPEED_NUM_100G,
+};
+
+static uint16_t
+nfp_net_link_speed_rte2nfp(uint16_t speed)
+{
+	uint16_t i;
+
+	for (i = 0; i < RTE_DIM(nfp_net_link_speed_nfp2rte); i++) {
+		if (speed == nfp_net_link_speed_nfp2rte[i])
+			return i;
+	}
+
+	return NFP_NET_CFG_STS_LINK_RATE_UNKNOWN;
+}
+
+static void
+nfp_net_notify_port_speed(struct rte_eth_dev *dev)
+{
+	struct nfp_net_hw *hw;
+	struct nfp_eth_table *eth_table;
+	uint32_t nn_link_status;
+
+	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	eth_table = hw->pf_dev->nfp_eth_table;
+
+	/**
+	 * Read the link status from NFP_NET_CFG_STS. If the link is down
+	 * then write the link speed NFP_NET_CFG_STS_LINK_RATE_UNKNOWN to
+	 * NFP_NET_CFG_STS_NSP_LINK_RATE.
+	 */
+	nn_link_status = nn_cfg_readw(hw, NFP_NET_CFG_STS);
+	if ((nn_link_status & NFP_NET_CFG_STS_LINK) == 0) {
+		nn_cfg_writew(hw, NFP_NET_CFG_STS_NSP_LINK_RATE, NFP_NET_CFG_STS_LINK_RATE_UNKNOWN);
+		return;
+	}
+	/**
+	 * Link is up so read the link speed from the eth_table and write to
+	 * NFP_NET_CFG_STS_NSP_LINK_RATE.
+	 */
+	nn_cfg_writew(hw, NFP_NET_CFG_STS_NSP_LINK_RATE,
+		      nfp_net_link_speed_rte2nfp(eth_table->ports[hw->idx].speed));
+}
+
 static int
 __nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t update)
 {
@@ -111,6 +163,9 @@ nfp_net_reconfig(struct nfp_net_hw *hw, uint32_t ctrl, uint32_t update)
 	PMD_DRV_LOG(DEBUG, "nfp_net_reconfig: ctrl=%08x update=%08x",
 		    ctrl, update);
 
+	if (hw->pf_dev != NULL && hw->pf_dev->app_fw_id == NFP_APP_FW_CORE_NIC)
+		nfp_net_notify_port_speed(hw->eth_dev);
+
 	rte_spinlock_lock(&hw->reconfig_lock);
 
 	nn_cfg_writel(hw, NFP_NET_CFG_CTRL, ctrl);
@@ -538,27 +593,15 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 {
 	struct nfp_net_hw *hw;
 	struct rte_eth_link link;
-	struct nfp_eth_table *nfp_eth_table;
-	uint32_t nn_link_status;
-	uint32_t i;
+	uint16_t nn_link_status;
 	int ret;
 
-	static const uint32_t ls_to_ethtool[] = {
-		[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = RTE_ETH_SPEED_NUM_NONE,
-		[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = RTE_ETH_SPEED_NUM_NONE,
-		[NFP_NET_CFG_STS_LINK_RATE_1G]          = RTE_ETH_SPEED_NUM_1G,
-		[NFP_NET_CFG_STS_LINK_RATE_10G]         = RTE_ETH_SPEED_NUM_10G,
-		[NFP_NET_CFG_STS_LINK_RATE_25G]         = RTE_ETH_SPEED_NUM_25G,
-		[NFP_NET_CFG_STS_LINK_RATE_40G]         = RTE_ETH_SPEED_NUM_40G,
-		[NFP_NET_CFG_STS_LINK_RATE_50G]         = RTE_ETH_SPEED_NUM_50G,
-		[NFP_NET_CFG_STS_LINK_RATE_100G]        = RTE_ETH_SPEED_NUM_100G,
-	};
-
 	PMD_DRV_LOG(DEBUG, "Link update");
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
-	nn_link_status = nn_cfg_readl(hw, NFP_NET_CFG_STS);
+	/* Read link status */
+	nn_link_status = nn_cfg_readw(hw, NFP_NET_CFG_STS);
 
 	memset(&link, 0, sizeof(struct rte_eth_link));
 
@@ -567,28 +610,17 @@ nfp_net_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complete)
 
 	link.link_duplex = RTE_ETH_LINK_FULL_DUPLEX;
 
-	if (hw->pf_dev != NULL) {
-		nfp_eth_table = hw->pf_dev->nfp_eth_table;
-		if (nfp_eth_table != NULL) {
-			link.link_speed = nfp_eth_table->ports[hw->idx].speed;
-			for (i = 0; i < RTE_DIM(ls_to_ethtool); i++) {
-				if (ls_to_ethtool[i] == link.link_speed)
-					break;
-			}
-			if (i == RTE_DIM(ls_to_ethtool))
-				link.link_speed = RTE_ETH_SPEED_NUM_NONE;
-		} else {
-			link.link_speed = RTE_ETH_SPEED_NUM_NONE;
-		}
-	} else {
-		nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
-				NFP_NET_CFG_STS_LINK_RATE_MASK;
+	/**
+	 * Shift and mask nn_link_status so that it is effectively the value
+	 * at offset NFP_NET_CFG_STS_NSP_LINK_RATE.
+	 */
+	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
+			 NFP_NET_CFG_STS_LINK_RATE_MASK;
 
-		if (nn_link_status >= RTE_DIM(ls_to_ethtool))
-			link.link_speed = RTE_ETH_SPEED_NUM_NONE;
-		else
-			link.link_speed = ls_to_ethtool[nn_link_status];
-	}
+	if (nn_link_status >= RTE_DIM(nfp_net_link_speed_nfp2rte))
+		link.link_speed = RTE_ETH_SPEED_NUM_NONE;
+	else
+		link.link_speed = nfp_net_link_speed_nfp2rte[nn_link_status];
 
 	ret = rte_eth_linkstatus_set(dev, &link);
 	if (ret == 0) {
diff --git a/drivers/net/nfp/nfp_ctrl.h b/drivers/net/nfp/nfp_ctrl.h
index 75e4458a1b..244048796c 100644
--- a/drivers/net/nfp/nfp_ctrl.h
+++ b/drivers/net/nfp/nfp_ctrl.h
@@ -178,6 +178,15 @@
 #define   NFP_NET_CFG_STS_LINK_RATE_40G           5
 #define   NFP_NET_CFG_STS_LINK_RATE_50G           6
 #define   NFP_NET_CFG_STS_LINK_RATE_100G          7
+
+/*
+ * NSP Link rate is a 16-bit word. It is no longer determined by
+ * firmware, instead it is read from the nfp_eth_table of the
+ * associated pf_dev and written to the NFP_NET_CFG_STS_NSP_LINK_RATE
+ * address by the PMD each time the port is reconfigured.
+ */
+#define NFP_NET_CFG_STS_NSP_LINK_RATE   0x0036
+
 #define NFP_NET_CFG_CAP                 0x0038
 #define NFP_NET_CFG_MAX_TXRINGS         0x003c
 #define NFP_NET_CFG_MAX_RXRINGS         0x0040
-- 
2.39.1

Re: [PATCH v2 2/2] net/nfp: write link speed to control BAR

[Ferruh Yigit]

On 3/10/2023 6:25 AM, Chaoyong He wrote:
> +	/**
> +	 * Shift and mask nn_link_status so that it is effectively the value
> +	 * at offset NFP_NET_CFG_STS_NSP_LINK_RATE.
> +	 */
> +	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> +			 NFP_NET_CFG_STS_LINK_RATE_MASK;

Thanks for extensive commenting, perhaps this is the source of
confusion, I can't see how above logic makes effectively the value at
offset LINK_RATE.

NFP_NET_CFG_STS_LINK_RATE_SHIFT = 1
NFP_NET_CFG_STS_LINK_RATE_MASK  = 0xF

NFP_NET_CFG_STS               = 0x34
NFP_NET_CFG_STS_NSP_LINK_RATE = 0x36

nfp_net_notify_port_speed()
	uint16_t speed;
	*(0x36) = speed

nfp_net_link_update()
	uint16_t val = *(0x34)
	val = (val >> 1) & 0xF;


How come 'speed' and 'val' are same value?

Either there is a mistake or FW is making something in the background, I
am trying to clarify this in past few comments but not able to yet.

RE: [PATCH v2 2/2] net/nfp: write link speed to control BAR

[Chaoyong He]

> On 3/10/2023 6:25 AM, Chaoyong He wrote:
> > +	/**
> > +	 * Shift and mask nn_link_status so that it is effectively the value
> > +	 * at offset NFP_NET_CFG_STS_NSP_LINK_RATE.
> > +	 */
> > +	nn_link_status = (nn_link_status >>
> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> > +			 NFP_NET_CFG_STS_LINK_RATE_MASK;
> 
> Thanks for extensive commenting, perhaps this is the source of confusion, I
> can't see how above logic makes effectively the value at offset LINK_RATE.
> 
> NFP_NET_CFG_STS_LINK_RATE_SHIFT = 1
> NFP_NET_CFG_STS_LINK_RATE_MASK  = 0xF
> 
> NFP_NET_CFG_STS               = 0x34
> NFP_NET_CFG_STS_NSP_LINK_RATE = 0x36
> 
> nfp_net_notify_port_speed()
> 	uint16_t speed;
> 	*(0x36) = speed
> 
> nfp_net_link_update()
> 	uint16_t val = *(0x34)
> 	val = (val >> 1) & 0xF;
> 
> 
> How come 'speed' and 'val' are same value?
> 
> Either there is a mistake or FW is making something in the background, I am
> trying to clarify this in past few comments but not able to yet.

Yes, here FW does do something in the background.
----------------------------------------------------------------------------

            16 bit write only                 16 bit read only

| x x x x x x x x x x x x y y y y | x x x x x x x x x x x y y y y x |

                                                 ^                                                ^

                                             0x36                                           0x34
------------------------------------------------------------------------------
The firmware confirm that whatever write into ‘yyyy’ field of 0x36 can be read from ‘yyyy’ field of 0x34.

Re: [PATCH v2 2/2] net/nfp: write link speed to control BAR

[Ferruh Yigit]

On 3/13/2023 3:03 AM, Chaoyong He wrote:
>> On 3/10/2023 6:25 AM, Chaoyong He wrote:
>>> +	/**
>>> +	 * Shift and mask nn_link_status so that it is effectively the value
>>> +	 * at offset NFP_NET_CFG_STS_NSP_LINK_RATE.
>>> +	 */
>>> +	nn_link_status = (nn_link_status >>
>> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
>>> +			 NFP_NET_CFG_STS_LINK_RATE_MASK;
>>
>> Thanks for extensive commenting, perhaps this is the source of confusion, I
>> can't see how above logic makes effectively the value at offset LINK_RATE.
>>
>> NFP_NET_CFG_STS_LINK_RATE_SHIFT = 1
>> NFP_NET_CFG_STS_LINK_RATE_MASK  = 0xF
>>
>> NFP_NET_CFG_STS               = 0x34
>> NFP_NET_CFG_STS_NSP_LINK_RATE = 0x36
>>
>> nfp_net_notify_port_speed()
>> 	uint16_t speed;
>> 	*(0x36) = speed
>>
>> nfp_net_link_update()
>> 	uint16_t val = *(0x34)
>> 	val = (val >> 1) & 0xF;
>>
>>
>> How come 'speed' and 'val' are same value?
>>
>> Either there is a mistake or FW is making something in the background, I am
>> trying to clarify this in past few comments but not able to yet.
> 
> Yes, here FW does do something in the background.
> ----------------------------------------------------------------------------
> 
>             16 bit write only                 16 bit read only
> 
> | x x x x x x x x x x x x y y y y | x x x x x x x x x x x y y y y x |
> 
>                                                  ^                                                ^
> 
>                                              0x36                                           0x34
> ------------------------------------------------------------------------------
> The firmware confirm that whatever write into ‘yyyy’ field of 0x36 can be read from ‘yyyy’ field of 0x34.
> 

Thanks, I was looking for this, I am progressing with the patch.

Re: [PATCH v2 0/2] write link speed to control BAR

[Ferruh Yigit]

On 3/10/2023 6:25 AM, Chaoyong He wrote:
> Due to changes in the firmware for NFPs, firmware will no longer write
> the link speed of a port to the control BAR. In line with the behaviour
> of the kernel NFP driver, this is now handled by the PMD by reading the
> value provided by the NSP in the nfp_eth_table struct within the pf_dev
> of the port and subsequently writing this value to the control BAR.
> 
> ---
> V2:
> * Using the 16bit read/write helper functions
> ---
> James Hershaw (2):
>   net/nfp: add helper functions for read/write 16b values
>   net/nfp: write link speed to control BAR

Series applied to dpdk-next-net/main, thanks.