Initialize basic mbx function.
Signed-off-by: Dong Yibo <dong100@mucse.com>
---
drivers/net/ethernet/mucse/rnpgbe/Makefile | 3 +-
drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 16 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 3 +
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 393 ++++++++++++++++++
.../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 25 ++
5 files changed, 439 insertions(+), 1 deletion(-)
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..5fc878ada4b1 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -6,4 +6,5 @@
obj-$(CONFIG_MGBE) += rnpgbe.o
rnpgbe-objs := rnpgbe_main.o\
- rnpgbe_chip.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 9a86e67d6395..7999bb99306b 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -5,6 +5,7 @@
#define _RNPGBE_H
#include <linux/types.h>
+#include <linux/mutex.h>
extern const struct rnpgbe_info rnpgbe_n500_info;
extern const struct rnpgbe_info rnpgbe_n210_info;
@@ -23,7 +24,22 @@ enum rnpgbe_hw_type {
rnpgbe_hw_unknown
};
+struct mucse_mbx_stats {
+ u32 msgs_tx;
+ u32 msgs_rx;
+ u32 acks;
+ u32 reqs;
+};
+
struct mucse_mbx_info {
+ struct mucse_mbx_stats stats;
+ u32 timeout;
+ u32 usec_delay;
+ u16 size;
+ u16 fw_req;
+ u16 fw_ack;
+ /* lock for only one use mbx */
+ struct mutex lock;
/* fw <--> pf mbx */
u32 fw_pf_shm_base;
u32 pf2fw_mbox_ctrl;
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index 179621ea09f3..f38daef752a3 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -1,8 +1,11 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+#include <linux/string.h>
+
#include "rnpgbe.h"
#include "rnpgbe_hw.h"
+#include "rnpgbe_mbx.h"
/**
* rnpgbe_init_common - Setup common attribute
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..856cd4c8ab6f
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
@@ -0,0 +1,393 @@
+// 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"
+
+/**
+ * mbx_data_rd32 - Reads reg with base mbx->fw_pf_shm_base
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ *
+ * Return: register value
+ **/
+static u32 mbx_data_rd32(struct mucse_mbx_info *mbx, u32 reg)
+{
+ struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+ return readl(hw->hw_addr + mbx->fw_pf_shm_base + reg);
+}
+
+/**
+ * mbx_data_wr32 - Writes value to reg with base mbx->fw_pf_shm_base
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ * @value: value to be written
+ *
+ **/
+static void mbx_data_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
+{
+ struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+ writel(value, hw->hw_addr + mbx->fw_pf_shm_base + reg);
+}
+
+/**
+ * mbx_ctrl_rd32 - Reads reg with base mbx->fw2pf_mbox_vec
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ *
+ * Return: register value
+ **/
+static u32 mbx_ctrl_rd32(struct mucse_mbx_info *mbx, u32 reg)
+{
+ struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+ return readl(hw->hw_addr + mbx->fw2pf_mbox_vec + reg);
+}
+
+/**
+ * mbx_ctrl_wr32 - Writes value to reg with base mbx->fw2pf_mbox_vec
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ * @value: value to be written
+ *
+ **/
+static void mbx_ctrl_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
+{
+ struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+ writel(value, hw->hw_addr + mbx->fw2pf_mbox_vec + reg);
+}
+
+/**
+ * mucse_mbx_get_fwreq - Read fw req from reg
+ * @mbx: pointer to the mbx structure
+ *
+ * Return: the fwreq value
+ **/
+static u16 mucse_mbx_get_fwreq(struct mucse_mbx_info *mbx)
+{
+ return mbx_data_rd32(mbx, MBX_FW2PF_COUNTER) & GENMASK_U32(15, 0);
+}
+
+/**
+ * mucse_mbx_get_fwack - Read fw ack from reg
+ * @mbx: pointer to the MBX structure
+ *
+ * Return: the fwack value
+ **/
+static u16 mucse_mbx_get_fwack(struct mucse_mbx_info *mbx)
+{
+ return (mbx_data_rd32(mbx, MBX_FW2PF_COUNTER) >> 16);
+}
+
+/**
+ * mucse_mbx_inc_pf_req - Increase req
+ * @hw: pointer to the HW structure
+ *
+ * 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)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u16 req;
+ u32 v;
+
+ v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
+ req = (v & GENMASK_U32(15, 0));
+ req++;
+ v &= GENMASK_U32(31, 16);
+ v |= req;
+ mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
+ hw->mbx.stats.msgs_tx++;
+}
+
+/**
+ * mucse_mbx_inc_pf_ack - Increase ack
+ * @hw: pointer to the HW structure
+ *
+ * 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)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u16 ack;
+ u32 v;
+
+ v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
+ ack = (v >> 16) & GENMASK_U32(15, 0);
+ ack++;
+ v &= GENMASK_U32(15, 0);
+ v |= (ack << 16);
+ mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
+ hw->mbx.stats.msgs_rx++;
+}
+
+/**
+ * mucse_check_for_msg_pf - Check to see if the fw has sent mail
+ * @hw: pointer to the HW structure
+ *
+ * Return: 0 if the fw has set the Status bit or else -EIO
+ **/
+static int mucse_check_for_msg_pf(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u16 hw_req_count;
+
+ hw_req_count = mucse_mbx_get_fwreq(mbx);
+ /* chip's register 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, we must check no-zero.
+ **/
+ if (hw_req_count != 0 && hw_req_count != hw->mbx.fw_req) {
+ hw->mbx.stats.reqs++;
+ return 0;
+ }
+
+ return -EIO;
+}
+
+/**
+ * mucse_poll_for_msg - Wait for message notification
+ * @hw: pointer to the HW structure
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+static int mucse_poll_for_msg(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int countdown = mbx->timeout;
+ int val;
+
+ return read_poll_timeout(mucse_check_for_msg_pf,
+ val, val == 0, mbx->usec_delay,
+ countdown * mbx->usec_delay,
+ false, hw);
+}
+
+/**
+ * mucse_check_for_ack_pf - Check to see if the VF has ACKed
+ * @hw: pointer to the HW structure
+ *
+ * Return: 0 if the fw has set the Status bit or else -EIO
+ **/
+static int mucse_check_for_ack_pf(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ u16 hw_fw_ack;
+
+ hw_fw_ack = mucse_mbx_get_fwack(mbx);
+ /* chip's register is reset to 0 when rc send reset
+ * mbx command. This causes 'hw_fw_ack != hw->mbx.fw_ack'
+ * be TRUE before fw really reply. Driver must wait fw reset
+ * done reply before using chip, we must check no-zero.
+ **/
+ if (hw_fw_ack != 0 && hw_fw_ack != hw->mbx.fw_ack) {
+ hw->mbx.stats.acks++;
+ return 0;
+ }
+
+ return -EIO;
+}
+
+/**
+ * mucse_poll_for_ack - Wait for message acknowledgment
+ * @hw: pointer to the HW structure
+ *
+ * Return: 0 if it successfully received a message acknowledgment
+ **/
+static int mucse_poll_for_ack(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int countdown = mbx->timeout;
+ int val;
+
+ return read_poll_timeout(mucse_check_for_ack_pf,
+ val, val == 0, mbx->usec_delay,
+ countdown * mbx->usec_delay,
+ false, hw);
+}
+
+/**
+ * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
+ * @hw: pointer to the HW structure
+ *
+ * This function maybe used in an irq handler.
+ *
+ * Return: 0 if we obtained the mailbox lock or else -EIO
+ **/
+static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int try_cnt = 5000;
+ u32 reg;
+
+ reg = PF2FW_MBOX_CTRL(mbx);
+ while (try_cnt-- > 0) {
+ mbx_ctrl_wr32(mbx, reg, MBOX_PF_HOLD);
+ /* force write back before check */
+ wmb();
+ if (mbx_ctrl_rd32(mbx, reg) & MBOX_PF_HOLD)
+ return 0;
+ udelay(100);
+ }
+ return -EIO;
+}
+
+/**
+ * mucse_read_mbx_pf - Read a message from the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * 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 fw request so no polling for message is needed.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int size_inwords = size / 4;
+ u32 ctrl_reg;
+ int ret;
+ int i;
+
+ ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+
+ ret = mucse_obtain_mbx_lock_pf(hw);
+ if (ret)
+ return ret;
+ for (i = 0; i < size_inwords; i++)
+ msg[i] = mbx_data_rd32(mbx, MBX_FW_PF_SHM_DATA + 4 * i);
+ /* Hw need write data_reg at last */
+ mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA, 0);
+ hw->mbx.fw_req = mucse_mbx_get_fwreq(mbx);
+ mucse_mbx_inc_pf_ack(hw);
+ mbx_ctrl_wr32(mbx, ctrl_reg, 0);
+
+ return 0;
+}
+
+/**
+ * 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
+ *
+ * Return: 0 if it successfully received a message notification and
+ * copied it into the receive buffer
+ **/
+int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ int ret;
+
+ ret = mucse_poll_for_msg(hw);
+ if (ret)
+ return ret;
+
+ return mucse_read_mbx_pf(hw, msg, size);
+}
+
+/**
+ * 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;
+ u32 v;
+
+ v = mbx_data_rd32(mbx, MBX_FW2PF_COUNTER);
+ hw->mbx.fw_req = v & GENMASK_U32(15, 0);
+ hw->mbx.fw_ack = (v >> 16) & GENMASK_U32(15, 0);
+ mbx_ctrl_wr32(mbx, PF2FW_MBOX_CTRL(mbx), 0);
+ mbx_ctrl_wr32(mbx, FW_PF_MBOX_MASK(mbx), GENMASK_U32(31, 16));
+}
+
+/**
+ * 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
+ */
+void mucse_init_mbx_params_pf(struct mucse_hw *hw)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+
+ mbx->usec_delay = 100;
+ mbx->timeout = (4 * USEC_PER_SEC) / mbx->usec_delay;
+ mbx->stats.msgs_tx = 0;
+ mbx->stats.msgs_rx = 0;
+ mbx->stats.reqs = 0;
+ mbx->stats.acks = 0;
+ mbx->size = MUCSE_MAILBOX_BYTES;
+ mutex_init(&mbx->lock);
+ mucse_mbx_reset(hw);
+}
+
+/**
+ * mucse_write_mbx_pf - Place a message in the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * This function maybe used in an irq handler.
+ *
+ * Return: 0 if it successfully copied message into the buffer
+ **/
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ struct mucse_mbx_info *mbx = &hw->mbx;
+ int size_inwords = size / 4;
+ u32 ctrl_reg;
+ int ret;
+ int i;
+
+ ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+ ret = mucse_obtain_mbx_lock_pf(hw);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < size_inwords; i++)
+ mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
+
+ /* flush msg and acks as we are overwriting the message buffer */
+ hw->mbx.fw_ack = mucse_mbx_get_fwack(mbx);
+ mucse_mbx_inc_pf_req(hw);
+ mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
+
+ return 0;
+}
+
+/**
+ * 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
+ *
+ * Return: 0 if it successfully copied message into the buffer and
+ * received an ack to that message within delay * timeout period
+ **/
+int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+ int ret;
+
+ ret = mucse_write_mbx_pf(hw, msg, size);
+ if (ret)
+ return ret;
+ return mucse_poll_for_ack(hw);
+}
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..110c1ee025ba
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_MBX_H
+#define _RNPGBE_MBX_H
+
+#include "rnpgbe.h"
+
+#define MUCSE_MAILBOX_BYTES 56
+#define MBX_FW2PF_COUNTER 0
+#define MBX_PF2FW_COUNTER 4
+#define MBX_FW_PF_SHM_DATA 8
+#define FW2PF_MBOX_VEC 0
+#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)
+#define MBOX_PF_HOLD BIT(3)
+#define MBOX_IRQ_EN 0
+#define MBOX_IRQ_DISABLE 1
+
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size);
+int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
+void mucse_init_mbx_params_pf(struct mucse_hw *hw);
+int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
+#endif /* _RNPGBE_MBX_H */
--
2.25.1> struct mucse_mbx_info {
> + struct mucse_mbx_stats stats;
> + u32 timeout;
> + u32 usec_delay;
> + u16 size;
> + u16 fw_req;
> + u16 fw_ack;
> + /* lock for only one use mbx */
> + struct mutex lock;
> /* fw <--> pf mbx */
> u32 fw_pf_shm_base;
> u32 pf2fw_mbox_ctrl;
> +/**
> + * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
> + * @hw: pointer to the HW structure
> + *
> + * This function maybe used in an irq handler.
> + *
> + * Return: 0 if we obtained the mailbox lock or else -EIO
> + **/
> +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + int try_cnt = 5000;
> + u32 reg;
> +
> + reg = PF2FW_MBOX_CTRL(mbx);
> + while (try_cnt-- > 0) {
> + mbx_ctrl_wr32(mbx, reg, MBOX_PF_HOLD);
> + /* force write back before check */
> + wmb();
> + if (mbx_ctrl_rd32(mbx, reg) & MBOX_PF_HOLD)
> + return 0;
> + udelay(100);
> + }
> + return -EIO;
> +}
If there is a function which obtains a lock, there is normally a
function which releases a lock. But i don't see it.
> +void mucse_init_mbx_params_pf(struct mucse_hw *hw)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> +
> + mbx->usec_delay = 100;
> + mbx->timeout = (4 * USEC_PER_SEC) / mbx->usec_delay;
> + mbx->stats.msgs_tx = 0;
> + mbx->stats.msgs_rx = 0;
> + mbx->stats.reqs = 0;
> + mbx->stats.acks = 0;
> + mbx->size = MUCSE_MAILBOX_BYTES;
> + mutex_init(&mbx->lock);
And this mutex never seems to be used anywhere. What is it supposed to
be protecting?
Andrew
---
pw-bot: cr
On Thu, Sep 04, 2025 at 12:24:17AM +0200, Andrew Lunn wrote:
> > struct mucse_mbx_info {
> > + struct mucse_mbx_stats stats;
> > + u32 timeout;
> > + u32 usec_delay;
> > + u16 size;
> > + u16 fw_req;
> > + u16 fw_ack;
> > + /* lock for only one use mbx */
> > + struct mutex lock;
> > /* fw <--> pf mbx */
> > u32 fw_pf_shm_base;
> > u32 pf2fw_mbox_ctrl;
>
> > +/**
> > + * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
> > + * @hw: pointer to the HW structure
> > + *
> > + * This function maybe used in an irq handler.
> > + *
> > + * Return: 0 if we obtained the mailbox lock or else -EIO
> > + **/
> > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + int try_cnt = 5000;
> > + u32 reg;
> > +
> > + reg = PF2FW_MBOX_CTRL(mbx);
> > + while (try_cnt-- > 0) {
> > + mbx_ctrl_wr32(mbx, reg, MBOX_PF_HOLD);
> > + /* force write back before check */
> > + wmb();
> > + if (mbx_ctrl_rd32(mbx, reg) & MBOX_PF_HOLD)
> > + return 0;
> > + udelay(100);
> > + }
> > + return -EIO;
> > +}
>
> If there is a function which obtains a lock, there is normally a
> function which releases a lock. But i don't see it.
>
The lock is relased when send MBOX_CTRL_REQ in mucse_write_mbx_pf:
mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
Set MBOX_PF_HOLD(bit3) to hold the lock, clear bit3 to release, and set
MBOX_CTRL_REQ(bit0) to send the req. req and lock are different bits in
one register. So we send the req along with releasing lock (set bit0 and
clear bit3).
Maybe I should add comment like this?
/* send the req along with releasing the lock */
mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
> > +void mucse_init_mbx_params_pf(struct mucse_hw *hw)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > +
> > + mbx->usec_delay = 100;
> > + mbx->timeout = (4 * USEC_PER_SEC) / mbx->usec_delay;
> > + mbx->stats.msgs_tx = 0;
> > + mbx->stats.msgs_rx = 0;
> > + mbx->stats.reqs = 0;
> > + mbx->stats.acks = 0;
> > + mbx->size = MUCSE_MAILBOX_BYTES;
> > + mutex_init(&mbx->lock);
>
> And this mutex never seems to be used anywhere. What is it supposed to
> be protecting?
>
mbx->lock is used in patch5, to ensure that only one uses mbx.
I will move it to patch5.
> Andrew
>
> ---
> pw-bot: cr
>
Thanks for your feedback.
On Thu, Sep 04, 2025 at 11:19:48AM +0800, Yibo Dong wrote:
> On Thu, Sep 04, 2025 at 12:24:17AM +0200, Andrew Lunn wrote:
> > > struct mucse_mbx_info {
> > > + struct mucse_mbx_stats stats;
> > > + u32 timeout;
> > > + u32 usec_delay;
> > > + u16 size;
> > > + u16 fw_req;
> > > + u16 fw_ack;
> > > + /* lock for only one use mbx */
> > > + struct mutex lock;
> > > /* fw <--> pf mbx */
> > > u32 fw_pf_shm_base;
> > > u32 pf2fw_mbox_ctrl;
> >
> > > +/**
> > > + * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
> > > + * @hw: pointer to the HW structure
> > > + *
> > > + * This function maybe used in an irq handler.
> > > + *
> > > + * Return: 0 if we obtained the mailbox lock or else -EIO
> > > + **/
> > > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
> > > +{
> > > + struct mucse_mbx_info *mbx = &hw->mbx;
> > > + int try_cnt = 5000;
> > > + u32 reg;
> > > +
> > > + reg = PF2FW_MBOX_CTRL(mbx);
> > > + while (try_cnt-- > 0) {
> > > + mbx_ctrl_wr32(mbx, reg, MBOX_PF_HOLD);
> > > + /* force write back before check */
> > > + wmb();
> > > + if (mbx_ctrl_rd32(mbx, reg) & MBOX_PF_HOLD)
> > > + return 0;
> > > + udelay(100);
> > > + }
> > > + return -EIO;
> > > +}
> >
> > If there is a function which obtains a lock, there is normally a
> > function which releases a lock. But i don't see it.
> >
>
> The lock is relased when send MBOX_CTRL_REQ in mucse_write_mbx_pf:
>
> mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
>
> Set MBOX_PF_HOLD(bit3) to hold the lock, clear bit3 to release, and set
> MBOX_CTRL_REQ(bit0) to send the req. req and lock are different bits in
> one register. So we send the req along with releasing lock (set bit0 and
> clear bit3).
> Maybe I should add comment like this?
>
> /* send the req along with releasing the lock */
> mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
As i said, functions like this come in pairs. obtain/release,
lock/unlock. When reading code, you want to be able to see both of the
pair in a function, to know the unlock is not missing. The kernel even
has tools which will validate all paths through a function releasing
locks. Often error paths get this wrong.
So please make this a function, give it a name which makes it obvious
it is the opposite of mucse_obtain_mbx_lock_pf().
Andrew
On Thu, Sep 04, 2025 at 02:05:57PM +0200, Andrew Lunn wrote:
> On Thu, Sep 04, 2025 at 11:19:48AM +0800, Yibo Dong wrote:
> > On Thu, Sep 04, 2025 at 12:24:17AM +0200, Andrew Lunn wrote:
> > > > struct mucse_mbx_info {
> > > > + struct mucse_mbx_stats stats;
> > > > + u32 timeout;
> > > > + u32 usec_delay;
> > > > + u16 size;
> > > > + u16 fw_req;
> > > > + u16 fw_ack;
> > > > + /* lock for only one use mbx */
> > > > + struct mutex lock;
> > > > /* fw <--> pf mbx */
> > > > u32 fw_pf_shm_base;
> > > > u32 pf2fw_mbox_ctrl;
> > >
> > > > +/**
> > > > + * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
> > > > + * @hw: pointer to the HW structure
> > > > + *
> > > > + * This function maybe used in an irq handler.
> > > > + *
> > > > + * Return: 0 if we obtained the mailbox lock or else -EIO
> > > > + **/
> > > > +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
> > > > +{
> > > > + struct mucse_mbx_info *mbx = &hw->mbx;
> > > > + int try_cnt = 5000;
> > > > + u32 reg;
> > > > +
> > > > + reg = PF2FW_MBOX_CTRL(mbx);
> > > > + while (try_cnt-- > 0) {
> > > > + mbx_ctrl_wr32(mbx, reg, MBOX_PF_HOLD);
> > > > + /* force write back before check */
> > > > + wmb();
> > > > + if (mbx_ctrl_rd32(mbx, reg) & MBOX_PF_HOLD)
> > > > + return 0;
> > > > + udelay(100);
> > > > + }
> > > > + return -EIO;
> > > > +}
> > >
> > > If there is a function which obtains a lock, there is normally a
> > > function which releases a lock. But i don't see it.
> > >
> >
> > The lock is relased when send MBOX_CTRL_REQ in mucse_write_mbx_pf:
> >
> > mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
> >
> > Set MBOX_PF_HOLD(bit3) to hold the lock, clear bit3 to release, and set
> > MBOX_CTRL_REQ(bit0) to send the req. req and lock are different bits in
> > one register. So we send the req along with releasing lock (set bit0 and
> > clear bit3).
> > Maybe I should add comment like this?
> >
> > /* send the req along with releasing the lock */
> > mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
>
> As i said, functions like this come in pairs. obtain/release,
> lock/unlock. When reading code, you want to be able to see both of the
> pair in a function, to know the unlock is not missing. The kernel even
> has tools which will validate all paths through a function releasing
> locks. Often error paths get this wrong.
>
> So please make this a function, give it a name which makes it obvious
> it is the opposite of mucse_obtain_mbx_lock_pf().
>
> Andrew
>
Got it, I will update this.
Thanks for your feedback.
On Wed, Sep 03, 2025 at 10:54:28AM +0800, Dong Yibo wrote:
> Initialize basic mbx function.
>
> Signed-off-by: Dong Yibo <dong100@mucse.com>
...
> +/**
> + * mucse_mbx_inc_pf_req - Increase req
> + * @hw: pointer to the HW structure
> + *
> + * 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)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u16 req;
> + u32 v;
> +
> + v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> + req = (v & GENMASK_U32(15, 0));
nit1: Unnecessary parentheses
nit2: I would have used FIELD_GET here in conjunction with something like.
#define MBX_PF2FW_COUNTER_MASK GENMASK_U32(15, 0)
> + req++;
> + v &= GENMASK_U32(31, 16);
> + v |= req;
And using FIELD_PREP is probably more succinct here.
Likewise in the following function
> + mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> + hw->mbx.stats.msgs_tx++;
> +}
> +
> +/**
> + * mucse_mbx_inc_pf_ack - Increase ack
> + * @hw: pointer to the HW structure
> + *
> + * 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)
> +{
> + struct mucse_mbx_info *mbx = &hw->mbx;
> + u16 ack;
> + u32 v;
> +
> + v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> + ack = (v >> 16) & GENMASK_U32(15, 0);
> + ack++;
> + v &= GENMASK_U32(15, 0);
> + v |= (ack << 16);
> + mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> + hw->mbx.stats.msgs_rx++;
> +}
...
> +/**
> + * 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;
> + u32 v;
> +
> + v = mbx_data_rd32(mbx, MBX_FW2PF_COUNTER);
> + hw->mbx.fw_req = v & GENMASK_U32(15, 0);
> + hw->mbx.fw_ack = (v >> 16) & GENMASK_U32(15, 0);
I'd use FIELD_GET here too.
> + mbx_ctrl_wr32(mbx, PF2FW_MBOX_CTRL(mbx), 0);
> + mbx_ctrl_wr32(mbx, FW_PF_MBOX_MASK(mbx), GENMASK_U32(31, 16));
> +}
...
On Wed, Sep 03, 2025 at 05:44:31PM +0100, Simon Horman wrote:
> On Wed, Sep 03, 2025 at 10:54:28AM +0800, Dong Yibo wrote:
> > Initialize basic mbx function.
> >
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
>
> ...
>
> > +/**
> > + * mucse_mbx_inc_pf_req - Increase req
> > + * @hw: pointer to the HW structure
> > + *
> > + * 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)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u16 req;
> > + u32 v;
> > +
> > + v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> > + req = (v & GENMASK_U32(15, 0));
>
> nit1: Unnecessary parentheses
> nit2: I would have used FIELD_GET here in conjunction with something like.
>
> #define MBX_PF2FW_COUNTER_MASK GENMASK_U32(15, 0)
>
Got it, maybe update it like this?
req = FIELD_GET(GENMASK_U32(15, 0), v);
> > + req++;
> > + v &= GENMASK_U32(31, 16);
> > + v |= req;
>
> And using FIELD_PREP is probably more succinct here.
>
> Likewise in the following function
>
Got it, maybe update it like this?
req++;
v = (v & ~GENMASK_U32(15, 0)) | FIELD_PREP(GENMASK_U32(15, 0), req);
> > + mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> > + hw->mbx.stats.msgs_tx++;
> > +}
> > +
> > +/**
> > + * mucse_mbx_inc_pf_ack - Increase ack
> > + * @hw: pointer to the HW structure
> > + *
> > + * 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)
> > +{
> > + struct mucse_mbx_info *mbx = &hw->mbx;
> > + u16 ack;
> > + u32 v;
> > +
> > + v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> > + ack = (v >> 16) & GENMASK_U32(15, 0);
> > + ack++;
> > + v &= GENMASK_U32(15, 0);
> > + v |= (ack << 16);
> > + mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> > + hw->mbx.stats.msgs_rx++;
> > +}
>
> ...
>
Maybe like this ?
...
v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
ack = FIELD_GET(GENMASK_U32(31, 16), v);
ack++;
v = (v & GENMASK_U32(15, 0)) | FIELD_PREP(GENMASK_U32(31, 16), ack);
mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
...
> > +/**
> > + * 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;
> > + u32 v;
> > +
> > + v = mbx_data_rd32(mbx, MBX_FW2PF_COUNTER);
> > + hw->mbx.fw_req = v & GENMASK_U32(15, 0);
> > + hw->mbx.fw_ack = (v >> 16) & GENMASK_U32(15, 0);
>
> I'd use FIELD_GET here too.
>
Maybe like this?
hw->mbx.fw_req = FIELD_GET(GENMASK_U32(15, 0), v);
hw->mbx.fw_ack = FIELD_GET(GENMASK_U32(31, 16), v);
> > + mbx_ctrl_wr32(mbx, PF2FW_MBOX_CTRL(mbx), 0);
> > + mbx_ctrl_wr32(mbx, FW_PF_MBOX_MASK(mbx), GENMASK_U32(31, 16));
> > +}
>
> ...
>
Thanks for your feedback.
On 03/09/2025 03:54, Dong Yibo wrote: > Initialize basic mbx function. > > Signed-off-by: Dong Yibo <dong100@mucse.com> > --- > drivers/net/ethernet/mucse/rnpgbe/Makefile | 3 +- > drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 16 + > .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 3 + > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 393 ++++++++++++++++++ > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 25 ++ > 5 files changed, 439 insertions(+), 1 deletion(-) > 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/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > index 179621ea09f3..f38daef752a3 100644 > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > @@ -1,8 +1,11 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Copyright(c) 2020 - 2025 Mucse Corporation. */ > > +#include <linux/string.h> I don't see a reason to have string.h included into rnpgbe_chip.c > + > #include "rnpgbe.h" > #include "rnpgbe_hw.h" > +#include "rnpgbe_mbx.h" I believe this part has to be done in the previous patch. Please, be sure that the code can compile after every patch in the patchset.
On Wed, Sep 03, 2025 at 11:53:17AM +0100, Vadim Fedorenko wrote: > On 03/09/2025 03:54, Dong Yibo wrote: > > Initialize basic mbx function. > > > > Signed-off-by: Dong Yibo <dong100@mucse.com> > > --- > > drivers/net/ethernet/mucse/rnpgbe/Makefile | 3 +- > > drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 16 + > > .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 3 + > > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 393 ++++++++++++++++++ > > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 25 ++ > > 5 files changed, 439 insertions(+), 1 deletion(-) > > 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/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > > index 179621ea09f3..f38daef752a3 100644 > > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > > @@ -1,8 +1,11 @@ > > // SPDX-License-Identifier: GPL-2.0 > > /* Copyright(c) 2020 - 2025 Mucse Corporation. */ > > +#include <linux/string.h> > > I don't see a reason to have string.h included into rnpgbe_chip.c > You are right, I should add it when it is used. > > + > > #include "rnpgbe.h" > > #include "rnpgbe_hw.h" > > +#include "rnpgbe_mbx.h" > > I believe this part has to be done in the previous patch. > Please, be sure that the code can compile after every patch in the > patchset. > You mean 'include "rnpgbe_mbx.h"'? But 'rnpgbe_mbx.h' is added in this patch. I had compiled every patch before submission for this series. And as you remind, I will keep check this in the future. Thanks for your feedback.
On 03/09/2025 12:12, Yibo Dong wrote: > On Wed, Sep 03, 2025 at 11:53:17AM +0100, Vadim Fedorenko wrote: >> On 03/09/2025 03:54, Dong Yibo wrote: >>> Initialize basic mbx function. >>> >>> Signed-off-by: Dong Yibo <dong100@mucse.com> >>> --- >>> drivers/net/ethernet/mucse/rnpgbe/Makefile | 3 +- >>> drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 16 + >>> .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 3 + >>> .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 393 ++++++++++++++++++ >>> .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 25 ++ >>> 5 files changed, 439 insertions(+), 1 deletion(-) >>> 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/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c >>> index 179621ea09f3..f38daef752a3 100644 >>> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c >>> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c >>> @@ -1,8 +1,11 @@ >>> // SPDX-License-Identifier: GPL-2.0 >>> /* Copyright(c) 2020 - 2025 Mucse Corporation. */ >>> +#include <linux/string.h> >> >> I don't see a reason to have string.h included into rnpgbe_chip.c >> > > You are right, I should add it when it is used. > >>> + >>> #include "rnpgbe.h" >>> #include "rnpgbe_hw.h" >>> +#include "rnpgbe_mbx.h" >> >> I believe this part has to be done in the previous patch. >> Please, be sure that the code can compile after every patch in the >> patchset. >> > > You mean 'include "rnpgbe_mbx.h"'? But 'rnpgbe_mbx.h' is added in this patch. Ok, so what's in rnpgbe_chip.c which needs rnpgbe_mbx.h to be included? If the change is introduced later in patch 5, the move this include to it as well. > I had compiled every patch before submission for this series. And as you > remind, I will keep check this in the future. > > Thanks for your feedback. >
On Wed, Sep 03, 2025 at 01:43:02PM +0100, Vadim Fedorenko wrote: > On 03/09/2025 12:12, Yibo Dong wrote: > > On Wed, Sep 03, 2025 at 11:53:17AM +0100, Vadim Fedorenko wrote: > > > On 03/09/2025 03:54, Dong Yibo wrote: > > > > Initialize basic mbx function. > > > > > > > > Signed-off-by: Dong Yibo <dong100@mucse.com> > > > > --- > > > > drivers/net/ethernet/mucse/rnpgbe/Makefile | 3 +- > > > > drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h | 16 + > > > > .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c | 3 + > > > > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c | 393 ++++++++++++++++++ > > > > .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h | 25 ++ > > > > 5 files changed, 439 insertions(+), 1 deletion(-) > > > > 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/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > > > > index 179621ea09f3..f38daef752a3 100644 > > > > --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > > > > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c > > > > @@ -1,8 +1,11 @@ > > > > // SPDX-License-Identifier: GPL-2.0 > > > > /* Copyright(c) 2020 - 2025 Mucse Corporation. */ > > > > +#include <linux/string.h> > > > > > > I don't see a reason to have string.h included into rnpgbe_chip.c > > > > > > > You are right, I should add it when it is used. > > > > > > + > > > > #include "rnpgbe.h" > > > > #include "rnpgbe_hw.h" > > > > +#include "rnpgbe_mbx.h" > > > > > > I believe this part has to be done in the previous patch. > > > Please, be sure that the code can compile after every patch in the > > > patchset. > > > > > > > You mean 'include "rnpgbe_mbx.h"'? But 'rnpgbe_mbx.h' is added in this patch. > > Ok, so what's in rnpgbe_chip.c which needs rnpgbe_mbx.h to be included? > If the change is introduced later in patch 5, the move this include to > it as well. > You are right, I will move it to patch 5. > > I had compiled every patch before submission for this series. And as you > > remind, I will keep check this in the future. > > > > Thanks for your feedback. > > > > Thanks for your feedback.
© 2016 - 2025 Red Hat, Inc.