[v2,2/2] net/nfp: write link speed to control BAR
Checks
Commit Message
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(-)
Comments
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.
> 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.
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.
@@ -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) {
@@ -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