Initialize basic mbx function.
Signed-off-by: Dong Yibo <dong100@mucse.com>
---
drivers/net/ethernet/mucse/rnpgbe/Makefile | 5 +-
drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 46 ++
.../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 5 +-
drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 2 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 1 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 623 ++++++++++++++++++
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 48 ++
7 files changed, 727 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
index 42c359f459d9..41177103b50c 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -5,5 +5,6 @@
#
obj-$(CONFIG_MGBE) += rnpgbe.o
-rnpgbe-objs := rnpgbe_main.o\
- rnpgbe_chip.o
+rnpgbe-objs := rnpgbe_main.o \
+ rnpgbe_chip.o \
+ rnpgbe_mbx.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 2ae836fc8951..46e2bb2fe71e 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -63,9 +63,51 @@ struct mucse_mac_info {
int clk_csr;
};
+struct mucse_hw;
+
+enum MBX_ID {
+ MBX_VF0 = 0,
+ MBX_VF1,
+ MBX_VF2,
+ MBX_VF3,
+ MBX_VF4,
+ MBX_VF5,
+ MBX_VF6,
+ MBX_VF7,
+ MBX_CM3CPU,
+ MBX_FW = MBX_CM3CPU,
+ MBX_VFCNT
+};
+
+struct mucse_mbx_operations {
+ void (*init_params)(struct mucse_hw *hw);
+ int (*read)(struct mucse_hw *hw, u32 *msg,
+ u16 size, enum MBX_ID id);
+ int (*write)(struct mucse_hw *hw, u32 *msg,
+ u16 size, enum MBX_ID id);
+ int (*read_posted)(struct mucse_hw *hw, u32 *msg,
+ u16 size, enum MBX_ID id);
+ int (*write_posted)(struct mucse_hw *hw, u32 *msg,
+ u16 size, enum MBX_ID id);
+ int (*check_for_msg)(struct mucse_hw *hw, enum MBX_ID id);
+ int (*check_for_ack)(struct mucse_hw *hw, enum MBX_ID id);
+ void (*configure)(struct mucse_hw *hw, int num_vec,
+ bool enable);
+};
+
+struct mucse_mbx_stats {
+ u32 msgs_tx;
+ u32 msgs_rx;
+ u32 acks;
+ u32 reqs;
+ u32 rsts;
+};
+
#define MAX_VF_NUM (8)
struct mucse_mbx_info {
+ struct mucse_mbx_operations ops;
+ struct mucse_mbx_stats stats;
u32 timeout;
u32 usec_delay;
u32 v2p_mailbox;
@@ -99,6 +141,8 @@ struct mucse_mbx_info {
int share_size;
};
+#include "rnpgbe_mbx.h"
+
struct mucse_hw {
void *back;
u8 pfvfnum;
@@ -110,6 +154,8 @@ struct mucse_hw {
u16 vendor_id;
u16 subsystem_device_id;
u16 subsystem_vendor_id;
+ int max_vfs;
+ int max_vfs_noari;
enum rnpgbe_hw_type hw_type;
struct mucse_dma_info dma;
struct mucse_eth_info eth;
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index 38c094965db9..b0e5fda632f3 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -6,6 +6,7 @@
#include "rnpgbe.h"
#include "rnpgbe_hw.h"
+#include "rnpgbe_mbx.h"
/**
* rnpgbe_get_invariants_n500 - setup for hw info
@@ -67,7 +68,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
mbx->fw_pf_mbox_mask = 0x2e200;
mbx->fw_vf_share_ram = 0x2b000;
mbx->share_size = 512;
-
+ memcpy(&hw->mbx.ops, &mucse_mbx_ops_generic, sizeof(hw->mbx.ops));
/* setup net feature here */
hw->feature_flags |= M_NET_FEATURE_SG |
M_NET_FEATURE_TX_CHECKSUM |
@@ -83,6 +84,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
M_NET_FEATURE_STAG_OFFLOAD;
/* start the default ahz, update later */
hw->usecstocount = 125;
+ hw->max_vfs = 7;
}
/**
@@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
/* update hw feature */
hw->feature_flags |= M_HW_FEATURE_EEE;
hw->usecstocount = 62;
+ hw->max_vfs_noari = 7;
}
const struct rnpgbe_info rnpgbe_n500_info = {
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index 2c7372a5e88d..ff7bd9b21550 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -14,6 +14,8 @@
#define RNPGBE_RING_BASE (0x1000)
#define RNPGBE_MAC_BASE (0x20000)
#define RNPGBE_ETH_BASE (0x10000)
+
+#define RNPGBE_DMA_DUMY (0x000c)
/* chip resourse */
#define RNPGBE_MAX_QUEUES (8)
/* multicast control table */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index 08f773199e9b..1e8360cae560 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -114,6 +114,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
hw->hw_addr = hw_addr;
hw->dma.dma_version = dma_version;
ii->get_invariants(hw);
+ hw->mbx.ops.init_params(hw);
return 0;
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
new file mode 100644
index 000000000000..56ace3057fea
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
@@ -0,0 +1,623 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2022 - 2025 Mucse Corporation. */
+
+#include <linux/pci.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/iopoll.h>
+#include "rnpgbe.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_hw.h"
+
+/**
+ * mucse_read_mbx - Reads a message from the mailbox
+ * @hw: Pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to read
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+
+ /* limit read to size of mailbox */
+ if (size > mbx->size)
+ size = mbx->size;
+
+ if (!mbx->ops.read)
+ return -EIO;
+
+ return mbx->ops.read(hw, msg, size, mbx_id);
+}
+
+/**
+ * mucse_write_mbx - Write a message to the mailbox
+ * @hw: Pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to write
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+
+ if (size > mbx->size)
+ return -EINVAL;
+
+ if (!mbx->ops.write)
+ return -EIO;
+
+ return mbx->ops.write(hw, msg, size, mbx_id);
+}
+
+/**
+ * mucse_mbx_get_req - Read req from reg
+ * @hw: Pointer to the HW structure
+ * @reg: Register to read
+ *
+ * @return: the req value
+ **/
+static u16 mucse_mbx_get_req(struct mucse_hw *hw, int reg)
+{
+ /* force memory barrier */
+ mb();
+ return ioread32(hw->hw_addr + reg) & GENMASK(15, 0);
+}
+
+/**
+ * mucse_mbx_get_ack - Read ack from reg
+ * @hw: Pointer to the HW structure
+ * @reg: Register to read
+ *
+ * @return: the ack value
+ **/
+static u16 mucse_mbx_get_ack(struct mucse_hw *hw, int reg)
+{
+ /* force memory barrier */
+ mb();
+ return (mbx_rd32(hw, reg) >> 16);
+}
+
+/**
+ * mucse_mbx_inc_pf_req - Increase req
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to read
+ *
+ * mucse_mbx_inc_pf_req read pf_req from hw, then write
+ * new value back after increase
+ **/
+static void mucse_mbx_inc_pf_req(struct mucse_hw *hw,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u32 reg, v;
+ u16 req;
+
+ reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
+ PF2VF_COUNTER(mbx, mbx_id);
+ v = mbx_rd32(hw, reg);
+ req = (v & GENMASK(15, 0));
+ req++;
+ v &= GENMASK(31, 16);
+ v |= req;
+ /* force before write to hw */
+ mb();
+ mbx_wr32(hw, reg, v);
+ /* update stats */
+ hw->mbx.stats.msgs_tx++;
+}
+
+/**
+ * mucse_mbx_inc_pf_ack - Increase ack
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to read
+ *
+ * mucse_mbx_inc_pf_ack read pf_ack from hw, then write
+ * new value back after increase
+ **/
+static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u32 reg, v;
+ u16 ack;
+
+ reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
+ PF2VF_COUNTER(mbx, mbx_id);
+ v = mbx_rd32(hw, reg);
+ ack = (v >> 16) & GENMASK(15, 0);
+ ack++;
+ v &= GENMASK(15, 0);
+ v |= (ack << 16);
+ /* force before write to hw */
+ mb();
+ mbx_wr32(hw, reg, v);
+ /* update stats */
+ hw->mbx.stats.msgs_rx++;
+}
+
+/**
+ * mucse_check_for_msg - Checks to see if vf/fw sent us mail
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+
+ if (!mbx->ops.check_for_msg)
+ return -EIO;
+
+ return mbx->ops.check_for_msg(hw, mbx_id);
+}
+
+/**
+ * mucse_check_for_ack - Checks to see if vf/fw sent us ACK
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+
+ if (!mbx->ops.check_for_ack)
+ return -EIO;
+ return mbx->ops.check_for_ack(hw, mbx_id);
+}
+
+/**
+ * mucse_poll_for_msg - Wait for message notification
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to poll
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_poll_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int countdown = mbx->timeout;
+ int val;
+
+ if (!countdown || !mbx->ops.check_for_msg)
+ return -EIO;
+
+ return read_poll_timeout(mbx->ops.check_for_msg,
+ val, val == 0, mbx->usec_delay,
+ countdown * mbx->usec_delay,
+ false, hw, mbx_id);
+}
+
+/**
+ * mucse_poll_for_ack - Wait for message acknowledgment
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to poll
+ *
+ * @return: 0 if it successfully received a message acknowledgment
+ **/
+static int mucse_poll_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int countdown = mbx->timeout;
+ int val;
+
+ if (!countdown || !mbx->ops.check_for_ack)
+ return -EIO;
+
+ return read_poll_timeout(mbx->ops.check_for_ack,
+ val, val == 0, mbx->usec_delay,
+ countdown * mbx->usec_delay,
+ false, hw, mbx_id);
+}
+
+/**
+ * mucse_read_posted_mbx - Wait for message notification and receive message
+ * @hw: Pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to read
+ *
+ * @return: 0 if it successfully received a message notification and
+ * copied it into the receive buffer.
+ **/
+static int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int ret_val;
+
+ if (!mbx->ops.read)
+ return -EIO;
+
+ ret_val = mucse_poll_for_msg(hw, mbx_id);
+
+ /* if ack received read message, otherwise we timed out */
+ if (!ret_val)
+ ret_val = mbx->ops.read(hw, msg, size, mbx_id);
+
+ return ret_val;
+}
+
+/**
+ * mucse_write_posted_mbx - Write a message to the mailbox, wait for ack
+ * @hw: Pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to write
+ *
+ * @return: 0 if it successfully copied message into the buffer and
+ * received an ack to that message within delay * timeout period
+ **/
+static int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int ret_val;
+
+ /* exit if either we can't write or there isn't a defined timeout */
+ if (!mbx->ops.write || !mbx->timeout)
+ return -EIO;
+
+ /* send msg and hold buffer lock */
+ ret_val = mbx->ops.write(hw, msg, size, mbx_id);
+
+ /* if msg sent wait until we receive an ack */
+ if (!ret_val)
+ ret_val = mucse_poll_for_ack(hw, mbx_id);
+
+ return ret_val;
+}
+
+/**
+ * mucse_check_for_msg_pf - checks to see if the vf/fw has sent mail
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 if the vf/fw has set the Status bit or else
+ * -EIO
+ **/
+static int mucse_check_for_msg_pf(struct mucse_hw *hw,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u16 hw_req_count = 0;
+ int ret_val = -EIO;
+
+ if (mbx_id == MBX_FW) {
+ hw_req_count = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
+ /* reg in hw should avoid 0 check */
+ if (mbx->mbx_feature & MBX_FEATURE_NO_ZERO) {
+ if (hw_req_count != 0 &&
+ hw_req_count != hw->mbx.fw_req) {
+ ret_val = 0;
+ hw->mbx.stats.reqs++;
+ }
+ } else {
+ if (hw_req_count != hw->mbx.fw_req) {
+ ret_val = 0;
+ hw->mbx.stats.reqs++;
+ }
+ }
+ } else {
+ if (mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
+ hw->mbx.vf_req[mbx_id]) {
+ ret_val = 0;
+ hw->mbx.stats.reqs++;
+ }
+ }
+
+ return ret_val;
+}
+
+/**
+ * mucse_check_for_ack_pf - checks to see if the VF has ACKed
+ * @hw: Pointer to the HW structure
+ * @mbx_id: Id of vf/fw to check
+ *
+ * @return: 0 if the vf/fw has set the Status bit or else
+ * -EIO
+ **/
+static int mucse_check_for_ack_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int ret_val = -EIO;
+ u16 hw_fw_ack;
+
+ if (mbx_id == MBX_FW) {
+ hw_fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
+ if (hw_fw_ack != 0 &&
+ hw_fw_ack != hw->mbx.fw_ack) {
+ ret_val = 0;
+ hw->mbx.stats.acks++;
+ }
+ } else {
+ if (mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
+ hw->mbx.vf_ack[mbx_id]) {
+ ret_val = 0;
+ hw->mbx.stats.acks++;
+ }
+ }
+
+ return ret_val;
+}
+
+/**
+ * mucse_obtain_mbx_lock_pf - obtain mailbox lock
+ * @hw: pointer to the HW structure
+ * @mbx_id: Id of vf/fw to obtain
+ *
+ * This function maybe used in an irq handler.
+ *
+ * @return: 0 if we obtained the mailbox lock
+ **/
+static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int try_cnt = 5000, ret;
+ u32 reg;
+
+ reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
+ PF2VF_MBOX_CTRL(mbx, mbx_id);
+ while (try_cnt-- > 0) {
+ /* Take ownership of the buffer */
+ mbx_wr32(hw, reg, MBOX_PF_HOLD);
+ /* force write back before check */
+ wmb();
+ if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
+ return 0;
+ udelay(100);
+ }
+ return ret;
+}
+
+/**
+ * mucse_write_mbx_pf - Places a message in the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to write
+ *
+ * This function maybe used in an irq handler.
+ *
+ * @return: 0 if it successfully copied message into the buffer
+ **/
+static int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u32 data_reg, ctrl_reg;
+ int ret_val = 0;
+ u16 i;
+
+ data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
+ PF_VF_SHM_DATA(mbx, mbx_id);
+ ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
+ PF2VF_MBOX_CTRL(mbx, mbx_id);
+ if (size > MUCSE_VFMAILBOX_SIZE)
+ return -EINVAL;
+
+ /* lock the mailbox to prevent pf/vf/fw race condition */
+ ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
+ if (ret_val)
+ goto out_no_write;
+
+ /* copy the caller specified message to the mailbox memory buffer */
+ for (i = 0; i < size; i++)
+ mbx_wr32(hw, data_reg + i * 4, msg[i]);
+
+ /* flush msg and acks as we are overwriting the message buffer */
+ if (mbx_id == MBX_FW) {
+ hw->mbx.fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
+ } else {
+ hw->mbx.vf_ack[mbx_id] =
+ mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id));
+ }
+ mucse_mbx_inc_pf_req(hw, mbx_id);
+
+ /* Interrupt VF/FW to tell it a message
+ * has been sent and release buffer
+ */
+ if (mbx->mbx_feature & MBX_FEATURE_WRITE_DELAY)
+ udelay(300);
+ mbx_wr32(hw, ctrl_reg, MBOX_CTRL_REQ);
+
+out_no_write:
+ return ret_val;
+}
+
+/**
+ * mucse_read_mbx_pf - Read a message from the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: The message buffer
+ * @size: Length of buffer
+ * @mbx_id: Id of vf/fw to read
+ *
+ * This function copies a message from the mailbox buffer to the caller's
+ * memory buffer. The presumption is that the caller knows that there was
+ * a message due to a vf/fw request so no polling for message is needed.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u32 data_reg, ctrl_reg;
+ int ret_val;
+ u32 i;
+
+ data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
+ PF_VF_SHM_DATA(mbx, mbx_id);
+ ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
+ PF2VF_MBOX_CTRL(mbx, mbx_id);
+
+ if (size > MUCSE_VFMAILBOX_SIZE)
+ return -EINVAL;
+ /* lock the mailbox to prevent pf/vf race condition */
+ ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
+ if (ret_val)
+ goto out_no_read;
+
+ /* we need this */
+ mb();
+ /* copy the message from the mailbox memory buffer */
+ for (i = 0; i < size; i++)
+ msg[i] = mbx_rd32(hw, data_reg + 4 * i);
+ mbx_wr32(hw, data_reg, 0);
+
+ /* update req */
+ if (mbx_id == MBX_FW) {
+ hw->mbx.fw_req = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
+ } else {
+ hw->mbx.vf_req[mbx_id] =
+ mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id));
+ }
+ /* Acknowledge receipt and release mailbox, then we're done */
+ mucse_mbx_inc_pf_ack(hw, mbx_id);
+ /* free ownership of the buffer */
+ mbx_wr32(hw, ctrl_reg, 0);
+
+out_no_read:
+ return ret_val;
+}
+
+/**
+ * mucse_mbx_reset - reset mbx info, sync info from regs
+ * @hw: Pointer to the HW structure
+ *
+ * This function reset all mbx variables to default.
+ **/
+static void mucse_mbx_reset(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int idx, v;
+
+ for (idx = 0; idx < hw->max_vfs; idx++) {
+ v = mbx_rd32(hw, VF2PF_COUNTER(mbx, idx));
+ hw->mbx.vf_req[idx] = v & GENMASK(15, 0);
+ hw->mbx.vf_ack[idx] = (v >> 16) & GENMASK(15, 0);
+ mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
+ }
+ v = mbx_rd32(hw, FW2PF_COUNTER(mbx));
+ hw->mbx.fw_req = v & GENMASK(15, 0);
+ hw->mbx.fw_ack = (v >> 16) & GENMASK(15, 0);
+
+ mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
+
+ if (PF_VF_MBOX_MASK_LO(mbx))
+ mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx), 0);
+ if (PF_VF_MBOX_MASK_HI(mbx))
+ mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx), 0);
+
+ mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), GENMASK(31, 16));
+}
+
+/**
+ * mucse_mbx_configure_pf - configure mbx to use nr_vec interrupt
+ * @hw: Pointer to the HW structure
+ * @nr_vec: Vector number for mbx
+ * @enable: TRUE for enable, FALSE for disable
+ *
+ * This function configure mbx to use interrupt nr_vec.
+ **/
+static void mucse_mbx_configure_pf(struct mucse_hw *hw, int nr_vec,
+ bool enable)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int idx = 0;
+ u32 v;
+
+ if (enable) {
+ for (idx = 0; idx < hw->max_vfs; idx++) {
+ v = mbx_rd32(hw, VF2PF_COUNTER(mbx, idx));
+ hw->mbx.vf_req[idx] = v & GENMASK(15, 0);
+ hw->mbx.vf_ack[idx] = (v >> 16) & GENMASK(15, 0);
+
+ mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
+ }
+ v = mbx_rd32(hw, FW2PF_COUNTER(mbx));
+ hw->mbx.fw_req = v & GENMASK(15, 0);
+ hw->mbx.fw_ack = (v >> 16) & GENMASK(15, 0);
+ mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
+
+ for (idx = 0; idx < hw->max_vfs; idx++) {
+ /* vf to pf req interrupt */
+ mbx_wr32(hw, VF2PF_MBOX_VEC(mbx, idx),
+ nr_vec);
+ }
+
+ if (PF_VF_MBOX_MASK_LO(mbx))
+ mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx), 0);
+ /* allow vf to vectors */
+
+ if (PF_VF_MBOX_MASK_HI(mbx))
+ mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx), 0);
+ /* enable irq */
+ /* bind fw mbx to irq */
+ mbx_wr32(hw, FW2PF_MBOX_VEC(mbx), nr_vec);
+ /* allow CM3FW to PF MBX IRQ */
+ mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), GENMASK(31, 16));
+ } else {
+ if (PF_VF_MBOX_MASK_LO(mbx))
+ mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx),
+ GENMASK(31, 0));
+ /* disable irq */
+ if (PF_VF_MBOX_MASK_HI(mbx))
+ mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx),
+ GENMASK(31, 0));
+
+ /* disable CM3FW to PF MBX IRQ */
+ mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), 0xfffffffe);
+
+ /* reset vf->pf status/ctrl */
+ for (idx = 0; idx < hw->max_vfs; idx++)
+ mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
+ /* reset pf->cm3 ctrl */
+ mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
+ /* used to sync link status */
+ mbx_wr32(hw, RNPGBE_DMA_DUMY, 0);
+ }
+}
+
+/**
+ * mucse_init_mbx_params_pf - set initial values for pf mailbox
+ * @hw: pointer to the HW structure
+ *
+ * Initializes the hw->mbx struct to correct values for pf mailbox
+ */
+static void mucse_init_mbx_params_pf(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+
+ mbx->usec_delay = 100;
+ mbx->timeout = (4 * 1000 * 1000) / mbx->usec_delay;
+ mbx->stats.msgs_tx = 0;
+ mbx->stats.msgs_rx = 0;
+ mbx->stats.reqs = 0;
+ mbx->stats.acks = 0;
+ mbx->stats.rsts = 0;
+ mbx->size = MUCSE_VFMAILBOX_SIZE;
+
+ mutex_init(&mbx->lock);
+ mucse_mbx_reset(hw);
+}
+
+struct mucse_mbx_operations mucse_mbx_ops_generic = {
+ .init_params = mucse_init_mbx_params_pf,
+ .read = mucse_read_mbx_pf,
+ .write = mucse_write_mbx_pf,
+ .read_posted = mucse_read_posted_mbx,
+ .write_posted = mucse_write_posted_mbx,
+ .check_for_msg = mucse_check_for_msg_pf,
+ .check_for_ack = mucse_check_for_ack_pf,
+ .configure = mucse_mbx_configure_pf,
+};
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
new file mode 100644
index 000000000000..0b4183e53e61
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_MBX_H
+#define _RNPGBE_MBX_H
+
+#include "rnpgbe.h"
+
+/* 14 words */
+#define MUCSE_VFMAILBOX_SIZE 14
+/* ================ PF <--> VF mailbox ================ */
+#define SHARE_MEM_BYTES 64
+static inline u32 PF_VF_SHM(struct mucse_mbx_info *mbx, int vf)
+{
+ return mbx->pf_vf_shm_base + mbx->mbx_mem_size * vf;
+}
+
+#define PF2VF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 0)
+#define VF2PF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 4)
+#define PF_VF_SHM_DATA(mbx, vf) (PF_VF_SHM(mbx, vf) + 8)
+#define VF2PF_MBOX_VEC(mbx, vf) ((mbx)->vf2pf_mbox_vec_base + 4 * (vf))
+#define PF2VF_MBOX_CTRL(mbx, vf) ((mbx)->pf2vf_mbox_ctrl_base + 4 * (vf))
+#define PF_VF_MBOX_MASK_LO(mbx) ((mbx)->pf_vf_mbox_mask_lo)
+#define PF_VF_MBOX_MASK_HI(mbx) ((mbx)->pf_vf_mbox_mask_hi)
+/* ================ PF <--> FW mailbox ================ */
+#define FW_PF_SHM(mbx) ((mbx)->fw_pf_shm_base)
+#define FW2PF_COUNTER(mbx) (FW_PF_SHM(mbx) + 0)
+#define PF2FW_COUNTER(mbx) (FW_PF_SHM(mbx) + 4)
+#define FW_PF_SHM_DATA(mbx) (FW_PF_SHM(mbx) + 8)
+#define FW2PF_MBOX_VEC(mbx) ((mbx)->fw2pf_mbox_vec)
+#define PF2FW_MBOX_CTRL(mbx) ((mbx)->pf2fw_mbox_ctrl)
+#define FW_PF_MBOX_MASK(mbx) ((mbx)->fw_pf_mbox_mask)
+#define MBOX_CTRL_REQ BIT(0) /* WO */
+#define MBOX_PF_HOLD (BIT(3)) /* VF:RO, PF:WR */
+#define MBOX_IRQ_EN 0
+#define MBOX_IRQ_DISABLE 1
+#define mbx_rd32(hw, reg) m_rd_reg((hw)->hw_addr + (reg))
+#define mbx_wr32(hw, reg, val) m_wr_reg((hw)->hw_addr + (reg), (val))
+
+extern struct mucse_mbx_operations mucse_mbx_ops_generic;
+
+int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id);
+int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
+ enum MBX_ID mbx_id);
+int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id);
+int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id);
+#endif /* _RNPGBE_MBX_H */
--
2.25.1On Mon, Jul 21, 2025 at 07:32:26PM +0800, Dong Yibo wrote:
> Initialize basic mbx function.
>
> Signed-off-by: Dong Yibo <dong100@mucse.com>
...
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
...
> +/**
> + * mucse_obtain_mbx_lock_pf - obtain mailbox lock
> + * @hw: pointer to the HW structure
> + * @mbx_id: Id of vf/fw to obtain
> + *
> + * This function maybe used in an irq handler.
> + *
> + * @return: 0 if we obtained the mailbox lock
> + **/
> +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int try_cnt = 5000, ret;
> + u32 reg;
> +
> + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> + PF2VF_MBOX_CTRL(mbx, mbx_id);
> + while (try_cnt-- > 0) {
> + /* Take ownership of the buffer */
> + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> + /* force write back before check */
> + wmb();
> + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> + return 0;
> + udelay(100);
> + }
> + return ret;
ret is declared, and returned here.
But it is never initialised.
Perhaps it is appropriate to return an error value here,
and update the kernel doc for this function accordingly.
Flagged by W=1 builds with Clang 20.1.8, and Smatch.
> +}
...
On Tue, Jul 22, 2025 at 12:35:42PM +0100, Simon Horman wrote:
> On Mon, Jul 21, 2025 at 07:32:26PM +0800, Dong Yibo wrote:
> > Initialize basic mbx function.
> >
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
>
> ...
>
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
>
> ...
>
> > +/**
> > + * mucse_obtain_mbx_lock_pf - obtain mailbox lock
> > + * @hw: pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to obtain
> > + *
> > + * This function maybe used in an irq handler.
> > + *
> > + * @return: 0 if we obtained the mailbox lock
> > + **/
> > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int try_cnt = 5000, ret;
> > + u32 reg;
> > +
> > + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > + PF2VF_MBOX_CTRL(mbx, mbx_id);
> > + while (try_cnt-- > 0) {
> > + /* Take ownership of the buffer */
> > + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> > + /* force write back before check */
> > + wmb();
> > + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> > + return 0;
> > + udelay(100);
> > + }
> > + return ret;
>
> ret is declared, and returned here.
> But it is never initialised.
>
> Perhaps it is appropriate to return an error value here,
> and update the kernel doc for this function accordingly.
>
> Flagged by W=1 builds with Clang 20.1.8, and Smatch.
>
> > +}
>
> ...
>
Got it, I will fix this.
Maybe my clang (10.0.0) is too old, I will update it and
try W=1 again.
Thanks for your feedback.
> > Flagged by W=1 builds with Clang 20.1.8, and Smatch. > > > > > +} > > > > ... > > > > Got it, I will fix this. > Maybe my clang (10.0.0) is too old, I will update it and > try W=1 again. 10.0.0 was released 24 Mar 2020. That is a five year old compiler! Try something a bit more modern. Andrew
On Wed, Jul 23, 2025 at 04:38:00PM +0200, Andrew Lunn wrote: > > > Flagged by W=1 builds with Clang 20.1.8, and Smatch. > > > > > > > +} > > > > > > ... > > > > > > > Got it, I will fix this. > > Maybe my clang (10.0.0) is too old, I will update it and > > try W=1 again. > > 10.0.0 was released 24 Mar 2020. That is a five year old compiler! > > Try something a bit more modern. > > Andrew > Ok, I have update it, and got the warning. Thanks for your feedback.
On 7/21/2025 4:32 AM, Dong Yibo wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Initialize basic mbx function.
>
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
> drivers/net/ethernet/mucse/rnpgbe/Makefile | 5 +-
> drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 46 ++
> .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 5 +-
> drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 2 +
> .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 1 +
> .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 623 ++++++++++++++++++
> .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 48 ++
> 7 files changed, 727 insertions(+), 3 deletions(-)
> create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
>
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> index 42c359f459d9..41177103b50c 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
> +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> @@ -5,5 +5,6 @@
> #
>
> obj-$(CONFIG_MGBE) += rnpgbe.o
> -rnpgbe-objs := rnpgbe_main.o\
> - rnpgbe_chip.o
> +rnpgbe-objs := rnpgbe_main.o \
> + rnpgbe_chip.o \
> + rnpgbe_mbx.o
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> index 2ae836fc8951..46e2bb2fe71e 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -63,9 +63,51 @@ struct mucse_mac_info {
> int clk_csr;
> };
>
> +struct mucse_hw;
> +
> +enum MBX_ID {
> + MBX_VF0 = 0,
> + MBX_VF1,
> + MBX_VF2,
> + MBX_VF3,
> + MBX_VF4,
> + MBX_VF5,
> + MBX_VF6,
> + MBX_VF7,
> + MBX_CM3CPU,
> + MBX_FW = MBX_CM3CPU,
> + MBX_VFCNT
> +};
> +
> +struct mucse_mbx_operations {
> + void (*init_params)(struct mucse_hw *hw);
> + int (*read)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*write)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*read_posted)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*write_posted)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*check_for_msg)(struct mucse_hw *hw, enum MBX_ID id);
> + int (*check_for_ack)(struct mucse_hw *hw, enum MBX_ID id);
> + void (*configure)(struct mucse_hw *hw, int num_vec,
> + bool enable);
> +};
> +
> +struct mucse_mbx_stats {
> + u32 msgs_tx;
> + u32 msgs_rx;
> + u32 acks;
> + u32 reqs;
> + u32 rsts;
> +};
> +
> #define MAX_VF_NUM (8)
>
> struct mucse_mbx_info {
> + struct mucse_mbx_operations ops;
> + struct mucse_mbx_stats stats;
> u32 timeout;
> u32 usec_delay;
> u32 v2p_mailbox;
> @@ -99,6 +141,8 @@ struct mucse_mbx_info {
> int share_size;
> };
>
> +#include "rnpgbe_mbx.h"
> +
> struct mucse_hw {
> void *back;
> u8 pfvfnum;
> @@ -110,6 +154,8 @@ struct mucse_hw {
> u16 vendor_id;
> u16 subsystem_device_id;
> u16 subsystem_vendor_id;
> + int max_vfs;
> + int max_vfs_noari;
> enum rnpgbe_hw_type hw_type;
> struct mucse_dma_info dma;
> struct mucse_eth_info eth;
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index 38c094965db9..b0e5fda632f3 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> @@ -6,6 +6,7 @@
>
> #include "rnpgbe.h"
> #include "rnpgbe_hw.h"
> +#include "rnpgbe_mbx.h"
>
> /**
> * rnpgbe_get_invariants_n500 - setup for hw info
> @@ -67,7 +68,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
> mbx->fw_pf_mbox_mask = 0x2e200;
> mbx->fw_vf_share_ram = 0x2b000;
> mbx->share_size = 512;
> -
> + memcpy(&hw->mbx.ops, &mucse_mbx_ops_generic, sizeof(hw->mbx.ops));
> /* setup net feature here */
> hw->feature_flags |= M_NET_FEATURE_SG |
> M_NET_FEATURE_TX_CHECKSUM |
> @@ -83,6 +84,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
> M_NET_FEATURE_STAG_OFFLOAD;
> /* start the default ahz, update later */
> hw->usecstocount = 125;
> + hw->max_vfs = 7;
> }
>
> /**
> @@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
> /* update hw feature */
> hw->feature_flags |= M_HW_FEATURE_EEE;
> hw->usecstocount = 62;
> + hw->max_vfs_noari = 7;
> }
>
> const struct rnpgbe_info rnpgbe_n500_info = {
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> index 2c7372a5e88d..ff7bd9b21550 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> @@ -14,6 +14,8 @@
> #define RNPGBE_RING_BASE (0x1000)
> #define RNPGBE_MAC_BASE (0x20000)
> #define RNPGBE_ETH_BASE (0x10000)
> +
> +#define RNPGBE_DMA_DUMY (0x000c)
> /* chip resourse */
> #define RNPGBE_MAX_QUEUES (8)
> /* multicast control table */
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 08f773199e9b..1e8360cae560 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -114,6 +114,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
> hw->hw_addr = hw_addr;
> hw->dma.dma_version = dma_version;
> ii->get_invariants(hw);
> + hw->mbx.ops.init_params(hw);
>
> return 0;
>
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> new file mode 100644
> index 000000000000..56ace3057fea
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> @@ -0,0 +1,623 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2022 - 2025 Mucse Corporation. */
> +
> +#include <linux/pci.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +#include "rnpgbe.h"
> +#include "rnpgbe_mbx.h"
> +#include "rnpgbe_hw.h"
> +
> +/**
> + * mucse_read_mbx - Reads a message from the mailbox
> + * @hw: Pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to read
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + /* limit read to size of mailbox */
> + if (size > mbx->size)
> + size = mbx->size;
This is just min(size, mbx->size). There's no need to open code min().
> +
> + if (!mbx->ops.read)
> + return -EIO;
> +
> + return mbx->ops.read(hw, msg, size, mbx_id);
> +}
> +
> +/**
> + * mucse_write_mbx - Write a message to the mailbox
> + * @hw: Pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to write
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + if (size > mbx->size)
> + return -EINVAL;
> +
> + if (!mbx->ops.write)
> + return -EIO;
> +
> + return mbx->ops.write(hw, msg, size, mbx_id);
> +}
> +
> +/**
> + * mucse_mbx_get_req - Read req from reg
> + * @hw: Pointer to the HW structure
> + * @reg: Register to read
> + *
> + * @return: the req value
> + **/
> +static u16 mucse_mbx_get_req(struct mucse_hw *hw, int reg)
> +{
> + /* force memory barrier */
> + mb();
> + return ioread32(hw->hw_addr + reg) & GENMASK(15, 0);
> +}
> +
> +/**
> + * mucse_mbx_get_ack - Read ack from reg
> + * @hw: Pointer to the HW structure
> + * @reg: Register to read
> + *
> + * @return: the ack value
> + **/
> +static u16 mucse_mbx_get_ack(struct mucse_hw *hw, int reg)
> +{
> + /* force memory barrier */
> + mb();
> + return (mbx_rd32(hw, reg) >> 16);
> +}
> +
> +/**
> + * mucse_mbx_inc_pf_req - Increase req
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to read
> + *
> + * mucse_mbx_inc_pf_req read pf_req from hw, then write
> + * new value back after increase
> + **/
> +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 reg, v;
> + u16 req;
> +
> + reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
> + PF2VF_COUNTER(mbx, mbx_id);
> + v = mbx_rd32(hw, reg);
> + req = (v & GENMASK(15, 0));
> + req++;
> + v &= GENMASK(31, 16);
> + v |= req;
> + /* force before write to hw */
> + mb();
> + mbx_wr32(hw, reg, v);
> + /* update stats */
Nit, but this comment is unnecessary. Same comment for all of the other
identical comments. They are just repeating "what" you are doing.
> + hw->mbx.stats.msgs_tx++;
> +}
> +
> +/**
> + * mucse_mbx_inc_pf_ack - Increase ack
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to read
> + *
> + * mucse_mbx_inc_pf_ack read pf_ack from hw, then write
> + * new value back after increase
> + **/
> +static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 reg, v;
> + u16 ack;
> +
> + reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
> + PF2VF_COUNTER(mbx, mbx_id);
> + v = mbx_rd32(hw, reg);
> + ack = (v >> 16) & GENMASK(15, 0);
> + ack++;
> + v &= GENMASK(15, 0);
> + v |= (ack << 16);
> + /* force before write to hw */
> + mb();
> + mbx_wr32(hw, reg, v);
> + /* update stats */
> + hw->mbx.stats.msgs_rx++;
> +}
> +
> +/**
> + * mucse_check_for_msg - Checks to see if vf/fw sent us mail
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to check
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + if (!mbx->ops.check_for_msg)
> + return -EIO;
> +
> + return mbx->ops.check_for_msg(hw, mbx_id);
> +}
> +
> +/**
> + * mucse_check_for_ack - Checks to see if vf/fw sent us ACK
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to check
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + if (!mbx->ops.check_for_ack)
> + return -EIO;
> + return mbx->ops.check_for_ack(hw, mbx_id);
> +}
> +
> +/**
> + * mucse_poll_for_msg - Wait for message notification
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to poll
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_poll_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int countdown = mbx->timeout;
> + int val;
> +
> + if (!countdown || !mbx->ops.check_for_msg)
> + return -EIO;
> +
> + return read_poll_timeout(mbx->ops.check_for_msg,
> + val, val == 0, mbx->usec_delay,
> + countdown * mbx->usec_delay,
> + false, hw, mbx_id);
> +}
> +
> +/**
> + * mucse_poll_for_ack - Wait for message acknowledgment
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to poll
> + *
> + * @return: 0 if it successfully received a message acknowledgment
> + **/
> +static int mucse_poll_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int countdown = mbx->timeout;
> + int val;
> +
> + if (!countdown || !mbx->ops.check_for_ack)
> + return -EIO;
> +
> + return read_poll_timeout(mbx->ops.check_for_ack,
> + val, val == 0, mbx->usec_delay,
> + countdown * mbx->usec_delay,
> + false, hw, mbx_id);
> +}
> +
> +/**
> + * mucse_read_posted_mbx - Wait for message notification and receive message
> + * @hw: Pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to read
> + *
> + * @return: 0 if it successfully received a message notification and
> + * copied it into the receive buffer.
> + **/
> +static int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int ret_val;
> +
> + if (!mbx->ops.read)
> + return -EIO;
> +
> + ret_val = mucse_poll_for_msg(hw, mbx_id);
> +
> + /* if ack received read message, otherwise we timed out */
> + if (!ret_val)
> + ret_val = mbx->ops.read(hw, msg, size, mbx_id);
> +
> + return ret_val;
IMO a more typical flow would be the following:
ret_val = mucse_poll_for_msg();
if (ret_val)
return ret_val;
return mbx->ops.read();
> +}
> +
> +/**
> + * mucse_write_posted_mbx - Write a message to the mailbox, wait for ack
> + * @hw: Pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to write
> + *
> + * @return: 0 if it successfully copied message into the buffer and
> + * received an ack to that message within delay * timeout period
> + **/
> +static int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int ret_val;
> +
> + /* exit if either we can't write or there isn't a defined timeout */
> + if (!mbx->ops.write || !mbx->timeout)
> + return -EIO;
> +
> + /* send msg and hold buffer lock */
> + ret_val = mbx->ops.write(hw, msg, size, mbx_id);
> +
> + /* if msg sent wait until we receive an ack */
> + if (!ret_val)
> + ret_val = mucse_poll_for_ack(hw, mbx_id);
> +
> + return ret_val;
IMO a more typical flow would be the following:
ret_val = mbx->ops.write();
if (ret_val)
return ret_val;
return mucse_poll_for_ack();
> +}
> +
> +/**
> + * mucse_check_for_msg_pf - checks to see if the vf/fw has sent mail
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to check
> + *
> + * @return: 0 if the vf/fw has set the Status bit or else
> + * -EIO
> + **/
> +static int mucse_check_for_msg_pf(struct mucse_hw *hw,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u16 hw_req_count = 0;
> + int ret_val = -EIO;
> +
> + if (mbx_id == MBX_FW) {
> + hw_req_count = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
> + /* reg in hw should avoid 0 check */
This comment explains "what" you are doing/preventing, but if the
comment is necessary it should explain "why" it's being done.
> + if (mbx->mbx_feature & MBX_FEATURE_NO_ZERO) {
> + if (hw_req_count != 0 &&
> + hw_req_count != hw->mbx.fw_req) {
> + ret_val = 0;
> + hw->mbx.stats.reqs++;
> + }
> + } else {
> + if (hw_req_count != hw->mbx.fw_req) {
> + ret_val = 0;
> + hw->mbx.stats.reqs++;
> + }
> + }
> + } else {
> + if (mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
> + hw->mbx.vf_req[mbx_id]) {
> + ret_val = 0;
> + hw->mbx.stats.reqs++;
> + }
> + }
> +
> + return ret_val;
Nit, but ret_val isn't really needed in this function. You can just
return 0 in all of the places where you set ret_val to 0. If you get
here you can "return -EIO".
> +}
> +
> +/**
> + * mucse_check_for_ack_pf - checks to see if the VF has ACKed
> + * @hw: Pointer to the HW structure
> + * @mbx_id: Id of vf/fw to check
> + *
> + * @return: 0 if the vf/fw has set the Status bit or else
> + * -EIO
> + **/
> +static int mucse_check_for_ack_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int ret_val = -EIO;
> + u16 hw_fw_ack;
> +
> + if (mbx_id == MBX_FW) {
> + hw_fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
> + if (hw_fw_ack != 0 &&
> + hw_fw_ack != hw->mbx.fw_ack) {
> + ret_val = 0;
> + hw->mbx.stats.acks++;
> + }
> + } else {
> + if (mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
> + hw->mbx.vf_ack[mbx_id]) {
> + ret_val = 0;
> + hw->mbx.stats.acks++;
> + }
> + }
> +
> + return ret_val;
Ditto on ret_val being unnecessary.
> +}
> +
> +/**
> + * mucse_obtain_mbx_lock_pf - obtain mailbox lock
> + * @hw: pointer to the HW structure
> + * @mbx_id: Id of vf/fw to obtain
> + *
> + * This function maybe used in an irq handler.
Nit, s/maybe/may be/
> + *
> + * @return: 0 if we obtained the mailbox lock
> + **/
> +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int try_cnt = 5000, ret;
> + u32 reg;
> +
> + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> + PF2VF_MBOX_CTRL(mbx, mbx_id);
> + while (try_cnt-- > 0) {
> + /* Take ownership of the buffer */
> + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> + /* force write back before check */
> + wmb();
> + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> + return 0;
> + udelay(100);
> + }
> + return ret;
Just as Andrew said, this is uninitialized. I don't think ret is needed
at all in this function.
Please think about whether local variables are needed or not in the rest
of the patches.
In this case you return 0 right away on success and can return
-ETIMEDOUT (or whatever is appropriate) on failure.
> +}
> +
> +/**
> + * mucse_write_mbx_pf - Places a message in the mailbox
> + * @hw: pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to write
> + *
> + * This function maybe used in an irq handler.
> + *
> + * @return: 0 if it successfully copied message into the buffer
> + **/
> +static int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 data_reg, ctrl_reg;
> + int ret_val = 0;
> + u16 i;
> +
> + data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
> + PF_VF_SHM_DATA(mbx, mbx_id);
> + ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> + PF2VF_MBOX_CTRL(mbx, mbx_id);
> + if (size > MUCSE_VFMAILBOX_SIZE)
> + return -EINVAL;
> +
> + /* lock the mailbox to prevent pf/vf/fw race condition */
> + ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
> + if (ret_val)
> + goto out_no_write;
Just return directly here. No need for a goto.
> +
> + /* copy the caller specified message to the mailbox memory buffer */
> + for (i = 0; i < size; i++)
> + mbx_wr32(hw, data_reg + i * 4, msg[i]);
> +
> + /* flush msg and acks as we are overwriting the message buffer */
> + if (mbx_id == MBX_FW) {
> + hw->mbx.fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
> + } else {
> + hw->mbx.vf_ack[mbx_id] =
> + mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id));
> + }
> + mucse_mbx_inc_pf_req(hw, mbx_id);
> +
> + /* Interrupt VF/FW to tell it a message
> + * has been sent and release buffer
> + */
> + if (mbx->mbx_feature & MBX_FEATURE_WRITE_DELAY)
> + udelay(300);
This delay seems arbitrary. How do you know 300us is sufficient?
> + mbx_wr32(hw, ctrl_reg, MBOX_CTRL_REQ);
> +
> +out_no_write:
This label isn't required, unless it's used in a future patch. If that's
the case introduce it in the future patch.
> + return ret_val;
> +}
> +
> +/**
> + * mucse_read_mbx_pf - Read a message from the mailbox
> + * @hw: pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to read
> + *
> + * This function copies a message from the mailbox buffer to the caller's
> + * memory buffer. The presumption is that the caller knows that there was
> + * a message due to a vf/fw request so no polling for message is needed.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 data_reg, ctrl_reg;
> + int ret_val;
> + u32 i;
> +
> + data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
> + PF_VF_SHM_DATA(mbx, mbx_id);
> + ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> + PF2VF_MBOX_CTRL(mbx, mbx_id);
> +
> + if (size > MUCSE_VFMAILBOX_SIZE)
> + return -EINVAL;
> + /* lock the mailbox to prevent pf/vf race condition */
> + ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
> + if (ret_val)
> + goto out_no_read;
> +
> + /* we need this */
This comment doesn't seem useful at all. Why do you need this comment?
> + mb();
> + /* copy the message from the mailbox memory buffer */
> + for (i = 0; i < size; i++)
> + msg[i] = mbx_rd32(hw, data_reg + 4 * i);
> + mbx_wr32(hw, data_reg, 0);
> +
> + /* update req */
> + if (mbx_id == MBX_FW) {
> + hw->mbx.fw_req = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
> + } else {
> + hw->mbx.vf_req[mbx_id] =
> + mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id));
> + }
> + /* Acknowledge receipt and release mailbox, then we're done */
> + mucse_mbx_inc_pf_ack(hw, mbx_id);
> + /* free ownership of the buffer */
> + mbx_wr32(hw, ctrl_reg, 0);
> +
> +out_no_read:
> + return ret_val;
> +}
> +
> +/**
> + * mucse_mbx_reset - reset mbx info, sync info from regs
> + * @hw: Pointer to the HW structure
> + *
> + * This function reset all mbx variables to default.
> + **/
> +static void mucse_mbx_reset(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int idx, v;
> +
> + for (idx = 0; idx < hw->max_vfs; idx++) {
> + v = mbx_rd32(hw, VF2PF_COUNTER(mbx, idx));
> + hw->mbx.vf_req[idx] = v & GENMASK(15, 0);
> + hw->mbx.vf_ack[idx] = (v >> 16) & GENMASK(15, 0);
> + mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
> + }
> + v = mbx_rd32(hw, FW2PF_COUNTER(mbx));
> + hw->mbx.fw_req = v & GENMASK(15, 0);
> + hw->mbx.fw_ack = (v >> 16) & GENMASK(15, 0);
> +
> + mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
> +
> + if (PF_VF_MBOX_MASK_LO(mbx))
> + mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx), 0);
> + if (PF_VF_MBOX_MASK_HI(mbx))
> + mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx), 0);
> +
> + mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), GENMASK(31, 16));
> +}
> +
> +/**
> + * mucse_mbx_configure_pf - configure mbx to use nr_vec interrupt
> + * @hw: Pointer to the HW structure
> + * @nr_vec: Vector number for mbx
> + * @enable: TRUE for enable, FALSE for disable
> + *
> + * This function configure mbx to use interrupt nr_vec.
> + **/
> +static void mucse_mbx_configure_pf(struct mucse_hw *hw, int nr_vec,
> + bool enable)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int idx = 0;
Nit, you don't need to initialize idx. It's always initialized before
it's used.
> + u32 v;
> +
> + if (enable) {
> + for (idx = 0; idx < hw->max_vfs; idx++) {
> + v = mbx_rd32(hw, VF2PF_COUNTER(mbx, idx));
> + hw->mbx.vf_req[idx] = v & GENMASK(15, 0);
> + hw->mbx.vf_ack[idx] = (v >> 16) & GENMASK(15, 0);
> +
> + mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
> + }
> + v = mbx_rd32(hw, FW2PF_COUNTER(mbx));
> + hw->mbx.fw_req = v & GENMASK(15, 0);
> + hw->mbx.fw_ack = (v >> 16) & GENMASK(15, 0);
> + mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
> +
> + for (idx = 0; idx < hw->max_vfs; idx++) {
> + /* vf to pf req interrupt */
> + mbx_wr32(hw, VF2PF_MBOX_VEC(mbx, idx),
> + nr_vec);
> + }
> +
> + if (PF_VF_MBOX_MASK_LO(mbx))
> + mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx), 0);
> + /* allow vf to vectors */
> +
> + if (PF_VF_MBOX_MASK_HI(mbx))
> + mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx), 0);
> + /* enable irq */
> + /* bind fw mbx to irq */
> + mbx_wr32(hw, FW2PF_MBOX_VEC(mbx), nr_vec);
> + /* allow CM3FW to PF MBX IRQ */
> + mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), GENMASK(31, 16));
> + } else {
> + if (PF_VF_MBOX_MASK_LO(mbx))
> + mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx),
> + GENMASK(31, 0));
> + /* disable irq */
> + if (PF_VF_MBOX_MASK_HI(mbx))
> + mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx),
> + GENMASK(31, 0));
> +
> + /* disable CM3FW to PF MBX IRQ */
> + mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), 0xfffffffe);
> +
> + /* reset vf->pf status/ctrl */
> + for (idx = 0; idx < hw->max_vfs; idx++)
> + mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
> + /* reset pf->cm3 ctrl */
> + mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
> + /* used to sync link status */
> + mbx_wr32(hw, RNPGBE_DMA_DUMY, 0);
> + }
> +}
> +
> +/**
> + * mucse_init_mbx_params_pf - set initial values for pf mailbox
> + * @hw: pointer to the HW structure
> + *
> + * Initializes the hw->mbx struct to correct values for pf mailbox
> + */
> +static void mucse_init_mbx_params_pf(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + mbx->usec_delay = 100;
> + mbx->timeout = (4 * 1000 * 1000) / mbx->usec_delay;
> + mbx->stats.msgs_tx = 0;
> + mbx->stats.msgs_rx = 0;
> + mbx->stats.reqs = 0;
> + mbx->stats.acks = 0;
> + mbx->stats.rsts = 0;
> + mbx->size = MUCSE_VFMAILBOX_SIZE;
> +
> + mutex_init(&mbx->lock);
> + mucse_mbx_reset(hw);
> +}
> +
> +struct mucse_mbx_operations mucse_mbx_ops_generic = {
> + .init_params = mucse_init_mbx_params_pf,
> + .read = mucse_read_mbx_pf,
> + .write = mucse_write_mbx_pf,
> + .read_posted = mucse_read_posted_mbx,
> + .write_posted = mucse_write_posted_mbx,
> + .check_for_msg = mucse_check_for_msg_pf,
> + .check_for_ack = mucse_check_for_ack_pf,
> + .configure = mucse_mbx_configure_pf,
> +};
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> new file mode 100644
> index 000000000000..0b4183e53e61
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#ifndef _RNPGBE_MBX_H
> +#define _RNPGBE_MBX_H
> +
> +#include "rnpgbe.h"
> +
> +/* 14 words */
> +#define MUCSE_VFMAILBOX_SIZE 14
Instead of the comment and the name, wouldn't it make more sense to
incorporate "words" into the define? Something like:
#define MUCSE_VFMAILBOX_WORDS 14
> +/* ================ PF <--> VF mailbox ================ */
> +#define SHARE_MEM_BYTES 64
> +static inline u32 PF_VF_SHM(struct mucse_mbx_info *mbx, int vf)
> +{
> + return mbx->pf_vf_shm_base + mbx->mbx_mem_size * vf;
> +}
> +
> +#define PF2VF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 0)
> +#define VF2PF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 4)
> +#define PF_VF_SHM_DATA(mbx, vf) (PF_VF_SHM(mbx, vf) + 8)
> +#define VF2PF_MBOX_VEC(mbx, vf) ((mbx)->vf2pf_mbox_vec_base + 4 * (vf))
> +#define PF2VF_MBOX_CTRL(mbx, vf) ((mbx)->pf2vf_mbox_ctrl_base + 4 * (vf))
> +#define PF_VF_MBOX_MASK_LO(mbx) ((mbx)->pf_vf_mbox_mask_lo)
> +#define PF_VF_MBOX_MASK_HI(mbx) ((mbx)->pf_vf_mbox_mask_hi)
> +/* ================ PF <--> FW mailbox ================ */
> +#define FW_PF_SHM(mbx) ((mbx)->fw_pf_shm_base)
> +#define FW2PF_COUNTER(mbx) (FW_PF_SHM(mbx) + 0)
> +#define PF2FW_COUNTER(mbx) (FW_PF_SHM(mbx) + 4)
> +#define FW_PF_SHM_DATA(mbx) (FW_PF_SHM(mbx) + 8)
> +#define FW2PF_MBOX_VEC(mbx) ((mbx)->fw2pf_mbox_vec)
> +#define PF2FW_MBOX_CTRL(mbx) ((mbx)->pf2fw_mbox_ctrl)
> +#define FW_PF_MBOX_MASK(mbx) ((mbx)->fw_pf_mbox_mask)
> +#define MBOX_CTRL_REQ BIT(0) /* WO */
> +#define MBOX_PF_HOLD (BIT(3)) /* VF:RO, PF:WR */
> +#define MBOX_IRQ_EN 0
> +#define MBOX_IRQ_DISABLE 1
> +#define mbx_rd32(hw, reg) m_rd_reg((hw)->hw_addr + (reg))
> +#define mbx_wr32(hw, reg, val) m_wr_reg((hw)->hw_addr + (reg), (val))
> +
> +extern struct mucse_mbx_operations mucse_mbx_ops_generic;
> +
> +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id);
> +int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id);
> +int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id);
> +int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id);
> +#endif /* _RNPGBE_MBX_H */
> --
> 2.25.1
>
>
On Mon, Jul 21, 2025 at 02:54:00PM -0700, Brett Creeley wrote:
> On 7/21/2025 4:32 AM, Dong Yibo wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> >
> >
> > Initialize basic mbx function.
> >
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
> > ---
> > drivers/net/ethernet/mucse/rnpgbe/Makefile | 5 +-
> > drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 46 ++
> > .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 5 +-
> > drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 2 +
> > .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 1 +
> > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 623 ++++++++++++++++++
> > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 48 ++
> > 7 files changed, 727 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> > create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> >
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> > index 42c359f459d9..41177103b50c 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> > @@ -5,5 +5,6 @@
> > #
> >
> > obj-$(CONFIG_MGBE) += rnpgbe.o
> > -rnpgbe-objs := rnpgbe_main.o\
> > - rnpgbe_chip.o
> > +rnpgbe-objs := rnpgbe_main.o \
> > + rnpgbe_chip.o \
> > + rnpgbe_mbx.o
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > index 2ae836fc8951..46e2bb2fe71e 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> > @@ -63,9 +63,51 @@ struct mucse_mac_info {
> > int clk_csr;
> > };
> >
> > +struct mucse_hw;
> > +
> > +enum MBX_ID {
> > + MBX_VF0 = 0,
> > + MBX_VF1,
> > + MBX_VF2,
> > + MBX_VF3,
> > + MBX_VF4,
> > + MBX_VF5,
> > + MBX_VF6,
> > + MBX_VF7,
> > + MBX_CM3CPU,
> > + MBX_FW = MBX_CM3CPU,
> > + MBX_VFCNT
> > +};
> > +
> > +struct mucse_mbx_operations {
> > + void (*init_params)(struct mucse_hw *hw);
> > + int (*read)(struct mucse_hw *hw, u32 *msg,
> > + u16 size, enum MBX_ID id);
> > + int (*write)(struct mucse_hw *hw, u32 *msg,
> > + u16 size, enum MBX_ID id);
> > + int (*read_posted)(struct mucse_hw *hw, u32 *msg,
> > + u16 size, enum MBX_ID id);
> > + int (*write_posted)(struct mucse_hw *hw, u32 *msg,
> > + u16 size, enum MBX_ID id);
> > + int (*check_for_msg)(struct mucse_hw *hw, enum MBX_ID id);
> > + int (*check_for_ack)(struct mucse_hw *hw, enum MBX_ID id);
> > + void (*configure)(struct mucse_hw *hw, int num_vec,
> > + bool enable);
> > +};
> > +
> > +struct mucse_mbx_stats {
> > + u32 msgs_tx;
> > + u32 msgs_rx;
> > + u32 acks;
> > + u32 reqs;
> > + u32 rsts;
> > +};
> > +
> > #define MAX_VF_NUM (8)
> >
> > struct mucse_mbx_info {
> > + struct mucse_mbx_operations ops;
> > + struct mucse_mbx_stats stats;
> > u32 timeout;
> > u32 usec_delay;
> > u32 v2p_mailbox;
> > @@ -99,6 +141,8 @@ struct mucse_mbx_info {
> > int share_size;
> > };
> >
> > +#include "rnpgbe_mbx.h"
> > +
> > struct mucse_hw {
> > void *back;
> > u8 pfvfnum;
> > @@ -110,6 +154,8 @@ struct mucse_hw {
> > u16 vendor_id;
> > u16 subsystem_device_id;
> > u16 subsystem_vendor_id;
> > + int max_vfs;
> > + int max_vfs_noari;
> > enum rnpgbe_hw_type hw_type;
> > struct mucse_dma_info dma;
> > struct mucse_eth_info eth;
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > index 38c094965db9..b0e5fda632f3 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> > @@ -6,6 +6,7 @@
> >
> > #include "rnpgbe.h"
> > #include "rnpgbe_hw.h"
> > +#include "rnpgbe_mbx.h"
> >
> > /**
> > * rnpgbe_get_invariants_n500 - setup for hw info
> > @@ -67,7 +68,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
> > mbx->fw_pf_mbox_mask = 0x2e200;
> > mbx->fw_vf_share_ram = 0x2b000;
> > mbx->share_size = 512;
> > -
> > + memcpy(&hw->mbx.ops, &mucse_mbx_ops_generic, sizeof(hw->mbx.ops));
> > /* setup net feature here */
> > hw->feature_flags |= M_NET_FEATURE_SG |
> > M_NET_FEATURE_TX_CHECKSUM |
> > @@ -83,6 +84,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
> > M_NET_FEATURE_STAG_OFFLOAD;
> > /* start the default ahz, update later */
> > hw->usecstocount = 125;
> > + hw->max_vfs = 7;
> > }
> >
> > /**
> > @@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
> > /* update hw feature */
> > hw->feature_flags |= M_HW_FEATURE_EEE;
> > hw->usecstocount = 62;
> > + hw->max_vfs_noari = 7;
> > }
> >
> > const struct rnpgbe_info rnpgbe_n500_info = {
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> > index 2c7372a5e88d..ff7bd9b21550 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> > @@ -14,6 +14,8 @@
> > #define RNPGBE_RING_BASE (0x1000)
> > #define RNPGBE_MAC_BASE (0x20000)
> > #define RNPGBE_ETH_BASE (0x10000)
> > +
> > +#define RNPGBE_DMA_DUMY (0x000c)
> > /* chip resourse */
> > #define RNPGBE_MAX_QUEUES (8)
> > /* multicast control table */
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > index 08f773199e9b..1e8360cae560 100644
> > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > @@ -114,6 +114,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
> > hw->hw_addr = hw_addr;
> > hw->dma.dma_version = dma_version;
> > ii->get_invariants(hw);
> > + hw->mbx.ops.init_params(hw);
> >
> > return 0;
> >
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> > new file mode 100644
> > index 000000000000..56ace3057fea
> > --- /dev/null
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> > @@ -0,0 +1,623 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright(c) 2022 - 2025 Mucse Corporation. */
> > +
> > +#include <linux/pci.h>
> > +#include <linux/errno.h>
> > +#include <linux/delay.h>
> > +#include <linux/iopoll.h>
> > +#include "rnpgbe.h"
> > +#include "rnpgbe_mbx.h"
> > +#include "rnpgbe_hw.h"
> > +
> > +/**
> > + * mucse_read_mbx - Reads a message from the mailbox
> > + * @hw: Pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to read
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + /* limit read to size of mailbox */
> > + if (size > mbx->size)
> > + size = mbx->size;
>
> This is just min(size, mbx->size). There's no need to open code min().
>
Got it. I will improve it.
> > +
> > + if (!mbx->ops.read)
> > + return -EIO;
> > +
> > + return mbx->ops.read(hw, msg, size, mbx_id);
> > +}
> > +
> > +/**
> > + * mucse_write_mbx - Write a message to the mailbox
> > + * @hw: Pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to write
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + if (size > mbx->size)
> > + return -EINVAL;
> > +
> > + if (!mbx->ops.write)
> > + return -EIO;
> > +
> > + return mbx->ops.write(hw, msg, size, mbx_id);
> > +}
> > +
> > +/**
> > + * mucse_mbx_get_req - Read req from reg
> > + * @hw: Pointer to the HW structure
> > + * @reg: Register to read
> > + *
> > + * @return: the req value
> > + **/
> > +static u16 mucse_mbx_get_req(struct mucse_hw *hw, int reg)
> > +{
> > + /* force memory barrier */
> > + mb();
> > + return ioread32(hw->hw_addr + reg) & GENMASK(15, 0);
> > +}
> > +
> > +/**
> > + * mucse_mbx_get_ack - Read ack from reg
> > + * @hw: Pointer to the HW structure
> > + * @reg: Register to read
> > + *
> > + * @return: the ack value
> > + **/
> > +static u16 mucse_mbx_get_ack(struct mucse_hw *hw, int reg)
> > +{
> > + /* force memory barrier */
> > + mb();
> > + return (mbx_rd32(hw, reg) >> 16);
> > +}
> > +
> > +/**
> > + * mucse_mbx_inc_pf_req - Increase req
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to read
> > + *
> > + * mucse_mbx_inc_pf_req read pf_req from hw, then write
> > + * new value back after increase
> > + **/
> > +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u32 reg, v;
> > + u16 req;
> > +
> > + reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
> > + PF2VF_COUNTER(mbx, mbx_id);
> > + v = mbx_rd32(hw, reg);
> > + req = (v & GENMASK(15, 0));
> > + req++;
> > + v &= GENMASK(31, 16);
> > + v |= req;
> > + /* force before write to hw */
> > + mb();
> > + mbx_wr32(hw, reg, v);
> > + /* update stats */
>
> Nit, but this comment is unnecessary. Same comment for all of the other
> identical comments. They are just repeating "what" you are doing.
>
Got it, I will try to check other patches.
> > + hw->mbx.stats.msgs_tx++;
> > +}
> > +
> > +/**
> > + * mucse_mbx_inc_pf_ack - Increase ack
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to read
> > + *
> > + * mucse_mbx_inc_pf_ack read pf_ack from hw, then write
> > + * new value back after increase
> > + **/
> > +static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u32 reg, v;
> > + u16 ack;
> > +
> > + reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
> > + PF2VF_COUNTER(mbx, mbx_id);
> > + v = mbx_rd32(hw, reg);
> > + ack = (v >> 16) & GENMASK(15, 0);
> > + ack++;
> > + v &= GENMASK(15, 0);
> > + v |= (ack << 16);
> > + /* force before write to hw */
> > + mb();
> > + mbx_wr32(hw, reg, v);
> > + /* update stats */
> > + hw->mbx.stats.msgs_rx++;
> > +}
> > +
> > +/**
> > + * mucse_check_for_msg - Checks to see if vf/fw sent us mail
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to check
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + if (!mbx->ops.check_for_msg)
> > + return -EIO;
> > +
> > + return mbx->ops.check_for_msg(hw, mbx_id);
> > +}
> > +
> > +/**
> > + * mucse_check_for_ack - Checks to see if vf/fw sent us ACK
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to check
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + if (!mbx->ops.check_for_ack)
> > + return -EIO;
> > + return mbx->ops.check_for_ack(hw, mbx_id);
> > +}
> > +
> > +/**
> > + * mucse_poll_for_msg - Wait for message notification
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to poll
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +static int mucse_poll_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int countdown = mbx->timeout;
> > + int val;
> > +
> > + if (!countdown || !mbx->ops.check_for_msg)
> > + return -EIO;
> > +
> > + return read_poll_timeout(mbx->ops.check_for_msg,
> > + val, val == 0, mbx->usec_delay,
> > + countdown * mbx->usec_delay,
> > + false, hw, mbx_id);
> > +}
> > +
> > +/**
> > + * mucse_poll_for_ack - Wait for message acknowledgment
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to poll
> > + *
> > + * @return: 0 if it successfully received a message acknowledgment
> > + **/
> > +static int mucse_poll_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int countdown = mbx->timeout;
> > + int val;
> > +
> > + if (!countdown || !mbx->ops.check_for_ack)
> > + return -EIO;
> > +
> > + return read_poll_timeout(mbx->ops.check_for_ack,
> > + val, val == 0, mbx->usec_delay,
> > + countdown * mbx->usec_delay,
> > + false, hw, mbx_id);
> > +}
> > +
> > +/**
> > + * mucse_read_posted_mbx - Wait for message notification and receive message
> > + * @hw: Pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to read
> > + *
> > + * @return: 0 if it successfully received a message notification and
> > + * copied it into the receive buffer.
> > + **/
> > +static int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int ret_val;
> > +
> > + if (!mbx->ops.read)
> > + return -EIO;
> > +
> > + ret_val = mucse_poll_for_msg(hw, mbx_id);
> > +
> > + /* if ack received read message, otherwise we timed out */
> > + if (!ret_val)
> > + ret_val = mbx->ops.read(hw, msg, size, mbx_id);
> > +
> > + return ret_val;
>
> IMO a more typical flow would be the following:
>
> ret_val = mucse_poll_for_msg();
> if (ret_val)
> return ret_val;
>
> return mbx->ops.read();
>
Got it, I will improve it.
> > +}
> > +
> > +/**
> > + * mucse_write_posted_mbx - Write a message to the mailbox, wait for ack
> > + * @hw: Pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to write
> > + *
> > + * @return: 0 if it successfully copied message into the buffer and
> > + * received an ack to that message within delay * timeout period
> > + **/
> > +static int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int ret_val;
> > +
> > + /* exit if either we can't write or there isn't a defined timeout */
> > + if (!mbx->ops.write || !mbx->timeout)
> > + return -EIO;
> > +
> > + /* send msg and hold buffer lock */
> > + ret_val = mbx->ops.write(hw, msg, size, mbx_id);
> > +
> > + /* if msg sent wait until we receive an ack */
> > + if (!ret_val)
> > + ret_val = mucse_poll_for_ack(hw, mbx_id);
> > +
> > + return ret_val;
>
>
> IMO a more typical flow would be the following:
>
> ret_val = mbx->ops.write();
> if (ret_val)
> return ret_val;
>
> return mucse_poll_for_ack();
>
Got it, I will improve it.
> > +}
> > +
> > +/**
> > + * mucse_check_for_msg_pf - checks to see if the vf/fw has sent mail
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to check
> > + *
> > + * @return: 0 if the vf/fw has set the Status bit or else
> > + * -EIO
> > + **/
> > +static int mucse_check_for_msg_pf(struct mucse_hw *hw,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u16 hw_req_count = 0;
> > + int ret_val = -EIO;
> > +
> > + if (mbx_id == MBX_FW) {
> > + hw_req_count = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
> > + /* reg in hw should avoid 0 check */
>
> This comment explains "what" you are doing/preventing, but if the comment is
> necessary it should explain "why" it's being done.
>
Got it, maybe like this?
Some chip's register FW2PF_COUNTER(mbx) is reset to 0 when rc send reset mbx
command. This causes 'hw_req_count != hw->mbx.fw_req' be true before fw really
reply. Driver must wait fw reset done reply before using chip.
> > + if (mbx->mbx_feature & MBX_FEATURE_NO_ZERO) {
> > + if (hw_req_count != 0 &&
> > + hw_req_count != hw->mbx.fw_req) {
> > + ret_val = 0;
> > + hw->mbx.stats.reqs++;
> > + }
> > + } else {
> > + if (hw_req_count != hw->mbx.fw_req) {
> > + ret_val = 0;
> > + hw->mbx.stats.reqs++;
> > + }
> > + }
> > + } else {
> > + if (mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
> > + hw->mbx.vf_req[mbx_id]) {
> > + ret_val = 0;
> > + hw->mbx.stats.reqs++;
> > + }
> > + }
> > +
> > + return ret_val;
>
> Nit, but ret_val isn't really needed in this function. You can just return 0
> in all of the places where you set ret_val to 0. If you get here you can
> "return -EIO".
>
Got it, I will improve it.
> > +}
> > +
> > +/**
> > + * mucse_check_for_ack_pf - checks to see if the VF has ACKed
> > + * @hw: Pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to check
> > + *
> > + * @return: 0 if the vf/fw has set the Status bit or else
> > + * -EIO
> > + **/
> > +static int mucse_check_for_ack_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int ret_val = -EIO;
> > + u16 hw_fw_ack;
> > +
> > + if (mbx_id == MBX_FW) {
> > + hw_fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
> > + if (hw_fw_ack != 0 &&
> > + hw_fw_ack != hw->mbx.fw_ack) {
> > + ret_val = 0;
> > + hw->mbx.stats.acks++;
> > + }
> > + } else {
> > + if (mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id)) !=
> > + hw->mbx.vf_ack[mbx_id]) {
> > + ret_val = 0;
> > + hw->mbx.stats.acks++;
> > + }
> > + }
> > +
> > + return ret_val;
>
> Ditto on ret_val being unnecessary.
>
Got it, I will improve it.
> > +}
> > +
> > +/**
> > + * mucse_obtain_mbx_lock_pf - obtain mailbox lock
> > + * @hw: pointer to the HW structure
> > + * @mbx_id: Id of vf/fw to obtain
> > + *
> > + * This function maybe used in an irq handler.
>
>
> Nit, s/maybe/may be/
>
Got it, I will fix it.
> > + *
> > + * @return: 0 if we obtained the mailbox lock
> > + **/
> > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int try_cnt = 5000, ret;
> > + u32 reg;
> > +
> > + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > + PF2VF_MBOX_CTRL(mbx, mbx_id);
> > + while (try_cnt-- > 0) {
> > + /* Take ownership of the buffer */
> > + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> > + /* force write back before check */
> > + wmb();
> > + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> > + return 0;
> > + udelay(100);
> > + }
> > + return ret;
>
> Just as Andrew said, this is uninitialized. I don't think ret is needed at
> all in this function.
>
> Please think about whether local variables are needed or not in the rest of
> the patches.
>
> In this case you return 0 right away on success and can return -ETIMEDOUT
> (or whatever is appropriate) on failure.
>
Yes, you are right. I will check rest of the patches.
> > +}
> > +
> > +/**
> > + * mucse_write_mbx_pf - Places a message in the mailbox
> > + * @hw: pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to write
> > + *
> > + * This function maybe used in an irq handler.
> > + *
> > + * @return: 0 if it successfully copied message into the buffer
> > + **/
> > +static int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u32 data_reg, ctrl_reg;
> > + int ret_val = 0;
> > + u16 i;
> > +
> > + data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
> > + PF_VF_SHM_DATA(mbx, mbx_id);
> > + ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > + PF2VF_MBOX_CTRL(mbx, mbx_id);
> > + if (size > MUCSE_VFMAILBOX_SIZE)
> > + return -EINVAL;
> > +
> > + /* lock the mailbox to prevent pf/vf/fw race condition */
> > + ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
> > + if (ret_val)
> > + goto out_no_write;
>
> Just return directly here. No need for a goto.
>
Got it, I will improve it.
> > +
> > + /* copy the caller specified message to the mailbox memory buffer */
> > + for (i = 0; i < size; i++)
> > + mbx_wr32(hw, data_reg + i * 4, msg[i]);
> > +
> > + /* flush msg and acks as we are overwriting the message buffer */
> > + if (mbx_id == MBX_FW) {
> > + hw->mbx.fw_ack = mucse_mbx_get_ack(hw, FW2PF_COUNTER(mbx));
> > + } else {
> > + hw->mbx.vf_ack[mbx_id] =
> > + mucse_mbx_get_ack(hw, VF2PF_COUNTER(mbx, mbx_id));
> > + }
> > + mucse_mbx_inc_pf_req(hw, mbx_id);
> > +
> > + /* Interrupt VF/FW to tell it a message
> > + * has been sent and release buffer
> > + */
> > + if (mbx->mbx_feature & MBX_FEATURE_WRITE_DELAY)
> > + udelay(300);
>
> This delay seems arbitrary. How do you know 300us is sufficient?
>
Yes, it is sufficient. But there is no explicit condition for it...
It is used to avoid timing issue in chip bus.
'Delay some time before write MBOX_CTRL_REQ reg' is required by chip
designer.
> > + mbx_wr32(hw, ctrl_reg, MBOX_CTRL_REQ);
> > +
> > +out_no_write:
>
> This label isn't required, unless it's used in a future patch. If that's the
> case introduce it in the future patch.
>
Got it, I will improve it.
> > + return ret_val;
> > +}
> > +
> > +/**
> > + * mucse_read_mbx_pf - Read a message from the mailbox
> > + * @hw: pointer to the HW structure
> > + * @msg: The message buffer
> > + * @size: Length of buffer
> > + * @mbx_id: Id of vf/fw to read
> > + *
> > + * This function copies a message from the mailbox buffer to the caller's
> > + * memory buffer. The presumption is that the caller knows that there was
> > + * a message due to a vf/fw request so no polling for message is needed.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u32 data_reg, ctrl_reg;
> > + int ret_val;
> > + u32 i;
> > +
> > + data_reg = (mbx_id == MBX_FW) ? FW_PF_SHM_DATA(mbx) :
> > + PF_VF_SHM_DATA(mbx, mbx_id);
> > + ctrl_reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > + PF2VF_MBOX_CTRL(mbx, mbx_id);
> > +
> > + if (size > MUCSE_VFMAILBOX_SIZE)
> > + return -EINVAL;
> > + /* lock the mailbox to prevent pf/vf race condition */
> > + ret_val = mucse_obtain_mbx_lock_pf(hw, mbx_id);
> > + if (ret_val)
> > + goto out_no_read;
> > +
> > + /* we need this */
>
> This comment doesn't seem useful at all. Why do you need this comment?
>
Yes, as Andrew pointed out, I should check 'mb()' here.
> > + mb();
> > + /* copy the message from the mailbox memory buffer */
> > + for (i = 0; i < size; i++)
> > + msg[i] = mbx_rd32(hw, data_reg + 4 * i);
> > + mbx_wr32(hw, data_reg, 0);
> > +
> > + /* update req */
> > + if (mbx_id == MBX_FW) {
> > + hw->mbx.fw_req = mucse_mbx_get_req(hw, FW2PF_COUNTER(mbx));
> > + } else {
> > + hw->mbx.vf_req[mbx_id] =
> > + mucse_mbx_get_req(hw, VF2PF_COUNTER(mbx, mbx_id));
> > + }
> > + /* Acknowledge receipt and release mailbox, then we're done */
> > + mucse_mbx_inc_pf_ack(hw, mbx_id);
> > + /* free ownership of the buffer */
> > + mbx_wr32(hw, ctrl_reg, 0);
> > +
> > +out_no_read:
> > + return ret_val;
> > +}
> > +
> > +/**
> > + * mucse_mbx_reset - reset mbx info, sync info from regs
> > + * @hw: Pointer to the HW structure
> > + *
> > + * This function reset all mbx variables to default.
> > + **/
> > +static void mucse_mbx_reset(struct mucse_hw *hw)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int idx, v;
> > +
> > + for (idx = 0; idx < hw->max_vfs; idx++) {
> > + v = mbx_rd32(hw, VF2PF_COUNTER(mbx, idx));
> > + hw->mbx.vf_req[idx] = v & GENMASK(15, 0);
> > + hw->mbx.vf_ack[idx] = (v >> 16) & GENMASK(15, 0);
> > + mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
> > + }
> > + v = mbx_rd32(hw, FW2PF_COUNTER(mbx));
> > + hw->mbx.fw_req = v & GENMASK(15, 0);
> > + hw->mbx.fw_ack = (v >> 16) & GENMASK(15, 0);
> > +
> > + mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
> > +
> > + if (PF_VF_MBOX_MASK_LO(mbx))
> > + mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx), 0);
> > + if (PF_VF_MBOX_MASK_HI(mbx))
> > + mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx), 0);
> > +
> > + mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), GENMASK(31, 16));
> > +}
> > +
> > +/**
> > + * mucse_mbx_configure_pf - configure mbx to use nr_vec interrupt
> > + * @hw: Pointer to the HW structure
> > + * @nr_vec: Vector number for mbx
> > + * @enable: TRUE for enable, FALSE for disable
> > + *
> > + * This function configure mbx to use interrupt nr_vec.
> > + **/
> > +static void mucse_mbx_configure_pf(struct mucse_hw *hw, int nr_vec,
> > + bool enable)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int idx = 0;
>
> Nit, you don't need to initialize idx. It's always initialized before it's
> used.
>
Got it, I will improve it.
> > + u32 v;
> > +
> > + if (enable) {
> > + for (idx = 0; idx < hw->max_vfs; idx++) {
> > + v = mbx_rd32(hw, VF2PF_COUNTER(mbx, idx));
> > + hw->mbx.vf_req[idx] = v & GENMASK(15, 0);
> > + hw->mbx.vf_ack[idx] = (v >> 16) & GENMASK(15, 0);
> > +
> > + mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
> > + }
> > + v = mbx_rd32(hw, FW2PF_COUNTER(mbx));
> > + hw->mbx.fw_req = v & GENMASK(15, 0);
> > + hw->mbx.fw_ack = (v >> 16) & GENMASK(15, 0);
> > + mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
> > +
> > + for (idx = 0; idx < hw->max_vfs; idx++) {
> > + /* vf to pf req interrupt */
> > + mbx_wr32(hw, VF2PF_MBOX_VEC(mbx, idx),
> > + nr_vec);
> > + }
> > +
> > + if (PF_VF_MBOX_MASK_LO(mbx))
> > + mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx), 0);
> > + /* allow vf to vectors */
> > +
> > + if (PF_VF_MBOX_MASK_HI(mbx))
> > + mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx), 0);
> > + /* enable irq */
> > + /* bind fw mbx to irq */
> > + mbx_wr32(hw, FW2PF_MBOX_VEC(mbx), nr_vec);
> > + /* allow CM3FW to PF MBX IRQ */
> > + mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), GENMASK(31, 16));
> > + } else {
> > + if (PF_VF_MBOX_MASK_LO(mbx))
> > + mbx_wr32(hw, PF_VF_MBOX_MASK_LO(mbx),
> > + GENMASK(31, 0));
> > + /* disable irq */
> > + if (PF_VF_MBOX_MASK_HI(mbx))
> > + mbx_wr32(hw, PF_VF_MBOX_MASK_HI(mbx),
> > + GENMASK(31, 0));
> > +
> > + /* disable CM3FW to PF MBX IRQ */
> > + mbx_wr32(hw, FW_PF_MBOX_MASK(mbx), 0xfffffffe);
> > +
> > + /* reset vf->pf status/ctrl */
> > + for (idx = 0; idx < hw->max_vfs; idx++)
> > + mbx_wr32(hw, PF2VF_MBOX_CTRL(mbx, idx), 0);
> > + /* reset pf->cm3 ctrl */
> > + mbx_wr32(hw, PF2FW_MBOX_CTRL(mbx), 0);
> > + /* used to sync link status */
> > + mbx_wr32(hw, RNPGBE_DMA_DUMY, 0);
> > + }
> > +}
> > +
> > +/**
> > + * mucse_init_mbx_params_pf - set initial values for pf mailbox
> > + * @hw: pointer to the HW structure
> > + *
> > + * Initializes the hw->mbx struct to correct values for pf mailbox
> > + */
> > +static void mucse_init_mbx_params_pf(struct mucse_hw *hw)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + mbx->usec_delay = 100;
> > + mbx->timeout = (4 * 1000 * 1000) / mbx->usec_delay;
> > + mbx->stats.msgs_tx = 0;
> > + mbx->stats.msgs_rx = 0;
> > + mbx->stats.reqs = 0;
> > + mbx->stats.acks = 0;
> > + mbx->stats.rsts = 0;
> > + mbx->size = MUCSE_VFMAILBOX_SIZE;
> > +
> > + mutex_init(&mbx->lock);
> > + mucse_mbx_reset(hw);
> > +}
> > +
> > +struct mucse_mbx_operations mucse_mbx_ops_generic = {
> > + .init_params = mucse_init_mbx_params_pf,
> > + .read = mucse_read_mbx_pf,
> > + .write = mucse_write_mbx_pf,
> > + .read_posted = mucse_read_posted_mbx,
> > + .write_posted = mucse_write_posted_mbx,
> > + .check_for_msg = mucse_check_for_msg_pf,
> > + .check_for_ack = mucse_check_for_ack_pf,
> > + .configure = mucse_mbx_configure_pf,
> > +};
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> > new file mode 100644
> > index 000000000000..0b4183e53e61
> > --- /dev/null
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> > @@ -0,0 +1,48 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> > +
> > +#ifndef _RNPGBE_MBX_H
> > +#define _RNPGBE_MBX_H
> > +
> > +#include "rnpgbe.h"
> > +
> > +/* 14 words */
> > +#define MUCSE_VFMAILBOX_SIZE 14
>
> Instead of the comment and the name, wouldn't it make more sense to
> incorporate "words" into the define? Something like:
>
> #define MUCSE_VFMAILBOX_WORDS 14
>
>
Got it, I will improve it.
> > +/* ================ PF <--> VF mailbox ================ */
> > +#define SHARE_MEM_BYTES 64
> > +static inline u32 PF_VF_SHM(struct mucse_mbx_info *mbx, int vf)
> > +{
> > + return mbx->pf_vf_shm_base + mbx->mbx_mem_size * vf;
> > +}
> > +
> > +#define PF2VF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 0)
> > +#define VF2PF_COUNTER(mbx, vf) (PF_VF_SHM(mbx, vf) + 4)
> > +#define PF_VF_SHM_DATA(mbx, vf) (PF_VF_SHM(mbx, vf) + 8)
> > +#define VF2PF_MBOX_VEC(mbx, vf) ((mbx)->vf2pf_mbox_vec_base + 4 * (vf))
> > +#define PF2VF_MBOX_CTRL(mbx, vf) ((mbx)->pf2vf_mbox_ctrl_base + 4 * (vf))
> > +#define PF_VF_MBOX_MASK_LO(mbx) ((mbx)->pf_vf_mbox_mask_lo)
> > +#define PF_VF_MBOX_MASK_HI(mbx) ((mbx)->pf_vf_mbox_mask_hi)
> > +/* ================ PF <--> FW mailbox ================ */
> > +#define FW_PF_SHM(mbx) ((mbx)->fw_pf_shm_base)
> > +#define FW2PF_COUNTER(mbx) (FW_PF_SHM(mbx) + 0)
> > +#define PF2FW_COUNTER(mbx) (FW_PF_SHM(mbx) + 4)
> > +#define FW_PF_SHM_DATA(mbx) (FW_PF_SHM(mbx) + 8)
> > +#define FW2PF_MBOX_VEC(mbx) ((mbx)->fw2pf_mbox_vec)
> > +#define PF2FW_MBOX_CTRL(mbx) ((mbx)->pf2fw_mbox_ctrl)
> > +#define FW_PF_MBOX_MASK(mbx) ((mbx)->fw_pf_mbox_mask)
> > +#define MBOX_CTRL_REQ BIT(0) /* WO */
> > +#define MBOX_PF_HOLD (BIT(3)) /* VF:RO, PF:WR */
> > +#define MBOX_IRQ_EN 0
> > +#define MBOX_IRQ_DISABLE 1
> > +#define mbx_rd32(hw, reg) m_rd_reg((hw)->hw_addr + (reg))
> > +#define mbx_wr32(hw, reg, val) m_wr_reg((hw)->hw_addr + (reg), (val))
> > +
> > +extern struct mucse_mbx_operations mucse_mbx_ops_generic;
> > +
> > +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id);
> > +int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id);
> > +int mucse_check_for_msg(struct mucse_hw *hw, enum MBX_ID mbx_id);
> > +int mucse_check_for_ack(struct mucse_hw *hw, enum MBX_ID mbx_id);
> > +#endif /* _RNPGBE_MBX_H */
> > --
> > 2.25.1
> >
> >
>
>
> #define MAX_VF_NUM (8)
> + hw->max_vfs = 7;
???
> }
>
> /**
> @@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
> /* update hw feature */
> hw->feature_flags |= M_HW_FEATURE_EEE;
> hw->usecstocount = 62;
> + hw->max_vfs_noari = 7;
???
> +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + /* limit read to size of mailbox */
> + if (size > mbx->size)
> + size = mbx->size;
> +
> + if (!mbx->ops.read)
> + return -EIO;
How would that happen?
> +
> + return mbx->ops.read(hw, msg, size, mbx_id);
> +int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + if (size > mbx->size)
> + return -EINVAL;
> +
> + if (!mbx->ops.write)
> + return -EIO;
How would either of these two conditions happen.
> +static u16 mucse_mbx_get_req(struct mucse_hw *hw, int reg)
> +{
> + /* force memory barrier */
> + mb();
> + return ioread32(hw->hw_addr + reg) & GENMASK(15, 0);
I'm no expert on memory barriers, but what are you trying to achieve
here? Probably the most used pattern of an mb() is to flush out writes
to hardware before doing a special write which triggers the hardware
to do something. That is not what is happening here.
> +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u32 reg, v;
> + u16 req;
> +
> + reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
> + PF2VF_COUNTER(mbx, mbx_id);
> + v = mbx_rd32(hw, reg);
> + req = (v & GENMASK(15, 0));
> + req++;
> + v &= GENMASK(31, 16);
> + v |= req;
> + /* force before write to hw */
> + mb();
> + mbx_wr32(hw, reg, v);
> + /* update stats */
> + hw->mbx.stats.msgs_tx++;
What are you forcing? As i said, i'm no expert on memory barriers, but
to me, it looks like whoever wrote this code also does not understand
memory barriers.
> +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int try_cnt = 5000, ret;
> + u32 reg;
> +
> + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> + PF2VF_MBOX_CTRL(mbx, mbx_id);
> + while (try_cnt-- > 0) {
> + /* Take ownership of the buffer */
> + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> + /* force write back before check */
> + wmb();
> + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> + return 0;
> + udelay(100);
> + }
> + return ret;
I've not compiled this, but isn't ret uninitialized here? I would also
expect it to return -ETIMEDOUT?
Andrew
On Mon, Jul 21, 2025 at 05:43:41PM +0200, Andrew Lunn wrote:
> > #define MAX_VF_NUM (8)
>
> > + hw->max_vfs = 7;
>
> ???
This is mistake, max vfs is 7. 8 is '7 vfs + 1 pf'.
>
>
> > }
> >
> > /**
> > @@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
> > /* update hw feature */
> > hw->feature_flags |= M_HW_FEATURE_EEE;
> > hw->usecstocount = 62;
> > + hw->max_vfs_noari = 7;
>
> ???
Bridge with no ari(Alternative Routing - ID Interpretation) function limits
8 function for one ep. This variable is used to limit vf numbers in no-ari
condition.
Of course, those not really used code should be removed in this patch.
>
> > +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + /* limit read to size of mailbox */
> > + if (size > mbx->size)
> > + size = mbx->size;
> > +
> > + if (!mbx->ops.read)
> > + return -EIO;
>
> How would that happen?
>
> > +
> > + return mbx->ops.read(hw, msg, size, mbx_id);
>
> > +int mucse_write_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + if (size > mbx->size)
> > + return -EINVAL;
> > +
> > + if (!mbx->ops.write)
> > + return -EIO;
>
> How would either of these two conditions happen.
>
Those are 'defensive code' which you point before. I should
remove those.
> > +static u16 mucse_mbx_get_req(struct mucse_hw *hw, int reg)
> > +{
> > + /* force memory barrier */
> > + mb();
> > + return ioread32(hw->hw_addr + reg) & GENMASK(15, 0);
>
> I'm no expert on memory barriers, but what are you trying to achieve
> here? Probably the most used pattern of an mb() is to flush out writes
> to hardware before doing a special write which triggers the hardware
> to do something. That is not what is happening here.
>
Got it, I will check and fix it.
> > +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw,
> > + enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u32 reg, v;
> > + u16 req;
> > +
> > + reg = (mbx_id == MBX_FW) ? PF2FW_COUNTER(mbx) :
> > + PF2VF_COUNTER(mbx, mbx_id);
> > + v = mbx_rd32(hw, reg);
> > + req = (v & GENMASK(15, 0));
> > + req++;
> > + v &= GENMASK(31, 16);
> > + v |= req;
> > + /* force before write to hw */
> > + mb();
> > + mbx_wr32(hw, reg, v);
> > + /* update stats */
> > + hw->mbx.stats.msgs_tx++;
>
> What are you forcing? As i said, i'm no expert on memory barriers, but
> to me, it looks like whoever wrote this code also does not understand
> memory barriers.
>
Got it, I will check and fix it.
> > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int try_cnt = 5000, ret;
> > + u32 reg;
> > +
> > + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > + PF2VF_MBOX_CTRL(mbx, mbx_id);
> > + while (try_cnt-- > 0) {
> > + /* Take ownership of the buffer */
> > + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> > + /* force write back before check */
> > + wmb();
> > + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> > + return 0;
> > + udelay(100);
> > + }
> > + return ret;
>
> I've not compiled this, but isn't ret uninitialized here? I would also
> expect it to return -ETIMEDOUT?
>
> Andrew
>
Yes, ret is uninitialized. I will fix this.
Thanks for your feedback.
On Tue, Jul 22, 2025 at 02:45:30PM +0800, Yibo Dong wrote:
> On Mon, Jul 21, 2025 at 05:43:41PM +0200, Andrew Lunn wrote:
> > > #define MAX_VF_NUM (8)
> >
> > > + hw->max_vfs = 7;
> >
> > ???
>
> This is mistake, max vfs is 7. 8 is '7 vfs + 1 pf'.
So it seems like you need to add a new #define for MAX_FUNCS_NUM, and
set MAX_VF_NUM to 7. And then actually use MAX_VP_NUM. When reviewing
your own code, seeing the number 7, not a define, should of been a
warning, something is wrong....
> > > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > > +{
> > > + struct mucse_mbx_info *mbx = &hw->mbx;
> > > + int try_cnt = 5000, ret;
> > > + u32 reg;
> > > +
> > > + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > > + PF2VF_MBOX_CTRL(mbx, mbx_id);
> > > + while (try_cnt-- > 0) {
> > > + /* Take ownership of the buffer */
> > > + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> > > + /* force write back before check */
> > > + wmb();
> > > + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> > > + return 0;
> > > + udelay(100);
> > > + }
> > > + return ret;
> >
> > I've not compiled this, but isn't ret uninitialized here? I would also
> > expect it to return -ETIMEDOUT?
> >
> > Andrew
> >
>
> Yes, ret is uninitialized. I will fix this.
Did the compiler give a warning? Code should be warning free. We also
expect networking code to be W=1 warning free.
Andrew
On Tue, Jul 22, 2025 at 03:50:01PM +0200, Andrew Lunn wrote:
> On Tue, Jul 22, 2025 at 02:45:30PM +0800, Yibo Dong wrote:
> > On Mon, Jul 21, 2025 at 05:43:41PM +0200, Andrew Lunn wrote:
> > > > #define MAX_VF_NUM (8)
> > >
> > > > + hw->max_vfs = 7;
> > >
> > > ???
> >
> > This is mistake, max vfs is 7. 8 is '7 vfs + 1 pf'.
>
> So it seems like you need to add a new #define for MAX_FUNCS_NUM, and
> set MAX_VF_NUM to 7. And then actually use MAX_VP_NUM. When reviewing
> your own code, seeing the number 7, not a define, should of been a
> warning, something is wrong....
>
Got it, I'll update this.
> > > > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw, enum MBX_ID mbx_id)
> > > > +{
> > > > + struct mucse_mbx_info *mbx = &hw->mbx;
> > > > + int try_cnt = 5000, ret;
> > > > + u32 reg;
> > > > +
> > > > + reg = (mbx_id == MBX_FW) ? PF2FW_MBOX_CTRL(mbx) :
> > > > + PF2VF_MBOX_CTRL(mbx, mbx_id);
> > > > + while (try_cnt-- > 0) {
> > > > + /* Take ownership of the buffer */
> > > > + mbx_wr32(hw, reg, MBOX_PF_HOLD);
> > > > + /* force write back before check */
> > > > + wmb();
> > > > + if (mbx_rd32(hw, reg) & MBOX_PF_HOLD)
> > > > + return 0;
> > > > + udelay(100);
> > > > + }
> > > > + return ret;
> > >
> > > I've not compiled this, but isn't ret uninitialized here? I would also
> > > expect it to return -ETIMEDOUT?
> > >
> > > Andrew
> > >
> >
> > Yes, ret is uninitialized. I will fix this.
>
> Did the compiler give a warning? Code should be warning free. We also
> expect networking code to be W=1 warning free.
>
> Andrew
>
I can get this warning with 'make CC=clang-16 W=1' now.
I did't make with clang before, I'll add this step for future patches.
Thanks for your feedback.
On 21/07/2025 12:32, Dong Yibo wrote:
> Initialize basic mbx function.
>
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
> drivers/net/ethernet/mucse/rnpgbe/Makefile | 5 +-
> drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 46 ++
> .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 5 +-
> drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 2 +
> .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c | 1 +
> .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 623 ++++++++++++++++++
> .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 48 ++
> 7 files changed, 727 insertions(+), 3 deletions(-)
> create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
>
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> index 42c359f459d9..41177103b50c 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
> +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> @@ -5,5 +5,6 @@
> #
>
> obj-$(CONFIG_MGBE) += rnpgbe.o
> -rnpgbe-objs := rnpgbe_main.o\
> - rnpgbe_chip.o
> +rnpgbe-objs := rnpgbe_main.o \
> + rnpgbe_chip.o \
> + rnpgbe_mbx.o
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> index 2ae836fc8951..46e2bb2fe71e 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -63,9 +63,51 @@ struct mucse_mac_info {
> int clk_csr;
> };
>
> +struct mucse_hw;
> +
> +enum MBX_ID {
> + MBX_VF0 = 0,
> + MBX_VF1,
> + MBX_VF2,
> + MBX_VF3,
> + MBX_VF4,
> + MBX_VF5,
> + MBX_VF6,
> + MBX_VF7,
> + MBX_CM3CPU,
> + MBX_FW = MBX_CM3CPU,
> + MBX_VFCNT
> +};
> +
> +struct mucse_mbx_operations {
> + void (*init_params)(struct mucse_hw *hw);
> + int (*read)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*write)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*read_posted)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*write_posted)(struct mucse_hw *hw, u32 *msg,
> + u16 size, enum MBX_ID id);
> + int (*check_for_msg)(struct mucse_hw *hw, enum MBX_ID id);
> + int (*check_for_ack)(struct mucse_hw *hw, enum MBX_ID id);
> + void (*configure)(struct mucse_hw *hw, int num_vec,
> + bool enable);
> +};
> +
> +struct mucse_mbx_stats {
> + u32 msgs_tx;
> + u32 msgs_rx;
> + u32 acks;
> + u32 reqs;
> + u32 rsts;
> +};
> +
> #define MAX_VF_NUM (8)
>
> struct mucse_mbx_info {
> + struct mucse_mbx_operations ops;
> + struct mucse_mbx_stats stats;
> u32 timeout;
> u32 usec_delay;
> u32 v2p_mailbox;
> @@ -99,6 +141,8 @@ struct mucse_mbx_info {
> int share_size;
> };
>
> +#include "rnpgbe_mbx.h"
> +
> struct mucse_hw {
> void *back;
> u8 pfvfnum;
> @@ -110,6 +154,8 @@ struct mucse_hw {
> u16 vendor_id;
> u16 subsystem_device_id;
> u16 subsystem_vendor_id;
> + int max_vfs;
> + int max_vfs_noari;
> enum rnpgbe_hw_type hw_type;
> struct mucse_dma_info dma;
> struct mucse_eth_info eth;
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index 38c094965db9..b0e5fda632f3 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> @@ -6,6 +6,7 @@
>
> #include "rnpgbe.h"
> #include "rnpgbe_hw.h"
> +#include "rnpgbe_mbx.h"
>
> /**
> * rnpgbe_get_invariants_n500 - setup for hw info
> @@ -67,7 +68,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
> mbx->fw_pf_mbox_mask = 0x2e200;
> mbx->fw_vf_share_ram = 0x2b000;
> mbx->share_size = 512;
> -
> + memcpy(&hw->mbx.ops, &mucse_mbx_ops_generic, sizeof(hw->mbx.ops));
that's bad pattern. it's better to have a constant set of callbacks per
device type and assign const pointer to it. It will make further debugs
much easier.
> /* setup net feature here */
> hw->feature_flags |= M_NET_FEATURE_SG |
> M_NET_FEATURE_TX_CHECKSUM |
> @@ -83,6 +84,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
> M_NET_FEATURE_STAG_OFFLOAD;
> /* start the default ahz, update later */
> hw->usecstocount = 125;
> + hw->max_vfs = 7;
> }
>
> /**
> @@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
> /* update hw feature */
> hw->feature_flags |= M_HW_FEATURE_EEE;
> hw->usecstocount = 62;
> + hw->max_vfs_noari = 7;
> }
>
> const struct rnpgbe_info rnpgbe_n500_info = {
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> index 2c7372a5e88d..ff7bd9b21550 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> @@ -14,6 +14,8 @@
> #define RNPGBE_RING_BASE (0x1000)
> #define RNPGBE_MAC_BASE (0x20000)
> #define RNPGBE_ETH_BASE (0x10000)
> +
> +#define RNPGBE_DMA_DUMY (0x000c)
> /* chip resourse */
> #define RNPGBE_MAX_QUEUES (8)
> /* multicast control table */
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 08f773199e9b..1e8360cae560 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -114,6 +114,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
> hw->hw_addr = hw_addr;
> hw->dma.dma_version = dma_version;
> ii->get_invariants(hw);
> + hw->mbx.ops.init_params(hw);
>
> return 0;
>
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> new file mode 100644
> index 000000000000..56ace3057fea
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> @@ -0,0 +1,623 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2022 - 2025 Mucse Corporation. */
> +
> +#include <linux/pci.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +#include "rnpgbe.h"
> +#include "rnpgbe_mbx.h"
> +#include "rnpgbe_hw.h"
> +
> +/**
> + * mucse_read_mbx - Reads a message from the mailbox
> + * @hw: Pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to read
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> + enum MBX_ID mbx_id)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + /* limit read to size of mailbox */
> + if (size > mbx->size)
> + size = mbx->size;
> +
> + if (!mbx->ops.read)
> + return -EIO;
is it even possible? you control the set of callbacks, and these
operations must be setup to have HW working. avoid defensive programming
here and in other places you use callbacks.
> +
> + return mbx->ops.read(hw, msg, size, mbx_id);
> +}
> +
[...]
© 2016 - 2025 Red Hat, Inc.